From e83c1f50359a7b00401c73894c7aa403f1ebd867 Mon Sep 17 00:00:00 2001 From: Taran Pelkey Date: Wed, 26 Aug 2026 16:29:03 -0500 Subject: [PATCH 1/3] Remove createuser deny in readonly --- policy/constants.go | 12 -------- policy/constants_test.go | 24 ++++++---------- policy/policy.go | 8 ++++-- policy/policy_regression_test.go | 48 +++++++++++++++++--------------- 4 files changed, 38 insertions(+), 54 deletions(-) diff --git a/policy/constants.go b/policy/constants.go index bfd02b3..fe07319 100644 --- a/policy/constants.go +++ b/policy/constants.go @@ -69,12 +69,6 @@ var DefaultPolicies = []struct { Actions: NewActionSet(GetBucketLocationAction, GetObjectAction), Resources: NewResourceSet(NewResource("*")), }, - { - SID: ID(""), - Effect: Deny, - Actions: NewActionSet(Action(CreateUserAdminAction)), - Resources: NewResourceSet(NewResource("*")), - }, }, }, }, @@ -91,12 +85,6 @@ var DefaultPolicies = []struct { Actions: NewActionSet(GetBucketLocationAction, GetObjectAction, ListBucketAction), Resources: NewResourceSet(NewResource("*")), }, - { - SID: ID(""), - Effect: Deny, - Actions: NewActionSet(Action(CreateUserAdminAction)), - Resources: NewResourceSet(NewResource("*")), - }, }, }, }, diff --git a/policy/constants_test.go b/policy/constants_test.go index 7458d18..38086bf 100644 --- a/policy/constants_test.go +++ b/policy/constants_test.go @@ -38,9 +38,8 @@ func TestDefaultPolicyReadOnly(t *testing.T) { } allowed := NewActionSet(GetBucketLocationAction, GetObjectAction) - denied := NewActionSet(Action(CreateUserAdminAction)) - var sawAllow, sawDeny bool + var sawAllow bool for _, s := range p.Statements { switch s.Effect { case Allow: @@ -49,14 +48,11 @@ func TestDefaultPolicyReadOnly(t *testing.T) { t.Errorf("readonly Allow actions = %v, want %v", s.Actions, allowed) } case Deny: - sawDeny = true - if !s.Actions.Equals(denied) { - t.Errorf("readonly Deny actions = %v, want %v", s.Actions, denied) - } + t.Errorf("readonly carries an unexpected Deny statement: %v", s.Actions) } } - if !sawAllow || !sawDeny { - t.Errorf("readonly missing Allow/Deny statement: allow=%v deny=%v", sawAllow, sawDeny) + if !sawAllow { + t.Error("readonly missing Allow statement") } } @@ -70,9 +66,8 @@ func TestDefaultPolicyConsoleReadOnly(t *testing.T) { } allowed := NewActionSet(GetBucketLocationAction, GetObjectAction, ListBucketAction) - denied := NewActionSet(Action(CreateUserAdminAction)) - var sawAllow, sawDeny bool + var sawAllow bool for _, s := range p.Statements { switch s.Effect { case Allow: @@ -81,14 +76,11 @@ func TestDefaultPolicyConsoleReadOnly(t *testing.T) { t.Errorf("consolereadonly Allow actions = %v, want %v", s.Actions, allowed) } case Deny: - sawDeny = true - if !s.Actions.Equals(denied) { - t.Errorf("consolereadonly Deny actions = %v, want %v", s.Actions, denied) - } + t.Errorf("consolereadonly carries an unexpected Deny statement: %v", s.Actions) } } - if !sawAllow || !sawDeny { - t.Errorf("consolereadonly missing Allow/Deny statement: allow=%v deny=%v", sawAllow, sawDeny) + if !sawAllow { + t.Error("consolereadonly missing Allow statement") } } diff --git a/policy/policy.go b/policy/policy.go index 00b951f..c59852a 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -180,9 +180,11 @@ func (iamp Policy) IsAllowedActions(bucketName, objectName string, conditionValu ObjectName: objectName, Action: admAction, ConditionValues: conditionValues, - // checks mainly for actions that can have explicit - // deny, while without it are implicitly enabled. - DenyOnly: action == CreateServiceAccountAdminAction || action == CreateUserAdminAction, + // Self-service actions the server grants implicitly and + // honors only an explicit Deny against, so the reported + // set has to be built the same way the server checks them. + DenyOnly: action == CreateServiceAccountAdminAction || + action == ChangeMyPasswordAdminAction, }) { actionSet.Add(admAction) } diff --git a/policy/policy_regression_test.go b/policy/policy_regression_test.go index 4592dcb..39daf7e 100644 --- a/policy/policy_regression_test.go +++ b/policy/policy_regression_test.go @@ -83,30 +83,32 @@ func TestHasDenyStatementOnStructLiteralPolicy(t *testing.T) { t.Error("struct literal policy carries a Deny but HasDenyStatement() reports false") } - checked := 0 - for _, name := range []string{"readonly", "consolereadonly", "diagnostics"} { - for _, dp := range DefaultPolicies { - if dp.Name != name { - continue - } - p := dp.Definition - hasDenyStmt := false - for _, s := range p.Statements { - if s.Effect == Deny { - hasDenyStmt = true - } - } - t.Logf("%-16s actual Deny statement=%v HasDenyStatement()=%v", name, hasDenyStmt, p.HasDenyStatement()) - if hasDenyStmt { - checked++ - if !p.HasDenyStatement() { - t.Errorf("%s: carries a Deny but HasDenyStatement() reports false", name) - } - } - } + // A Deny buried behind an Allow still has to be found, since the field the + // naive implementation trusted is only ever set by the parse path. + mixed := Policy{ + Version: DefaultVersion, + Statements: []Statement{ + NewStatement("", Allow, NewActionSet(GetObjectAction), + NewResourceSet(NewResource("*")), condition.NewFunctions()), + NewStatement("", Deny, NewActionSet(Action(CreateUserAdminAction)), + NewResourceSet(NewResource("*")), condition.NewFunctions()), + }, + } + if !mixed.HasDenyStatement() { + t.Error("struct literal policy with a trailing Deny but HasDenyStatement() reports false") + } + + // The negative case matters just as much: reporting a Deny that is not + // there would send callers down the slow evaluation path for every policy. + allowOnly := Policy{ + Version: DefaultVersion, + Statements: []Statement{ + NewStatement("", Allow, NewActionSet(GetObjectAction), + NewResourceSet(NewResource("*")), condition.NewFunctions()), + }, } - if checked == 0 { - t.Error("none of the named canned policies carries a Deny; this test checked nothing") + if allowOnly.HasDenyStatement() { + t.Error("struct literal policy carries no Deny but HasDenyStatement() reports true") } } From e4ff6461a08cc25fc0129edd6b9beaebe4855f78 Mon Sep 17 00:00:00 2001 From: Taran Pelkey Date: Wed, 26 Aug 2026 16:44:07 -0500 Subject: [PATCH 2/3] fix regression test --- policy/policy_regression_test.go | 76 +++++++++++++++++--------------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/policy/policy_regression_test.go b/policy/policy_regression_test.go index 39daf7e..95f36f5 100644 --- a/policy/policy_regression_test.go +++ b/policy/policy_regression_test.go @@ -68,47 +68,53 @@ func TestDropDuplicateStatementsKeepsNotResources(t *testing.T) { // Policy.hasDeny is only set by updateActionIndex, which a policy assembled as // a struct literal outside this package never reaches, so HasDenyStatement must -// not trust the field alone. +// not trust the field alone. Every policy below is built as a literal here, so +// none of them has been through a parse path at all. func TestHasDenyStatementOnStructLiteralPolicy(t *testing.T) { - // A literal built here has been through no parse path at all, so it pins - // the behavior down regardless of what the canned policies contain. - literal := Policy{ - Version: DefaultVersion, - Statements: []Statement{ - NewStatement("", Deny, NewActionSet(GetObjectAction), - NewResourceSet(NewResource("*")), condition.NewFunctions()), - }, - } - if !literal.HasDenyStatement() { - t.Error("struct literal policy carries a Deny but HasDenyStatement() reports false") + stmt := func(effect Effect, action Action) Statement { + return NewStatement("", effect, NewActionSet(action), + NewResourceSet(NewResource("*")), condition.NewFunctions()) } - // A Deny buried behind an Allow still has to be found, since the field the - // naive implementation trusted is only ever set by the parse path. - mixed := Policy{ - Version: DefaultVersion, - Statements: []Statement{ - NewStatement("", Allow, NewActionSet(GetObjectAction), - NewResourceSet(NewResource("*")), condition.NewFunctions()), - NewStatement("", Deny, NewActionSet(Action(CreateUserAdminAction)), - NewResourceSet(NewResource("*")), condition.NewFunctions()), + tests := []struct { + name string + statements []Statement + want bool + }{ + { + // Pins the behavior regardless of what the canned policies contain. + name: "deny only", + statements: []Statement{stmt(Deny, GetObjectAction)}, + want: true, }, - } - if !mixed.HasDenyStatement() { - t.Error("struct literal policy with a trailing Deny but HasDenyStatement() reports false") - } - - // The negative case matters just as much: reporting a Deny that is not - // there would send callers down the slow evaluation path for every policy. - allowOnly := Policy{ - Version: DefaultVersion, - Statements: []Statement{ - NewStatement("", Allow, NewActionSet(GetObjectAction), - NewResourceSet(NewResource("*")), condition.NewFunctions()), + { + // A Deny buried behind an Allow still has to be found, since the + // field the naive implementation trusted is only ever set by the + // parse path. + name: "allow then deny", + statements: []Statement{ + stmt(Allow, GetObjectAction), + stmt(Deny, Action(CreateUserAdminAction)), + }, + want: true, + }, + { + // The negative case matters just as much: reporting a Deny that is + // not there would send callers down the slow evaluation path for + // every policy. + name: "allow only", + statements: []Statement{stmt(Allow, GetObjectAction)}, + want: false, }, } - if allowOnly.HasDenyStatement() { - t.Error("struct literal policy carries no Deny but HasDenyStatement() reports true") + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := Policy{Version: DefaultVersion, Statements: tt.statements} + if got := p.HasDenyStatement(); got != tt.want { + t.Errorf("HasDenyStatement() = %v, want %v", got, tt.want) + } + }) } } From 2cae2261918efadd747b645200d21dd16ba394b4 Mon Sep 17 00:00:00 2001 From: Taran Pelkey Date: Thu, 27 Aug 2026 10:23:15 -0500 Subject: [PATCH 3/3] Add new regression test --- policy/policy_regression_test.go | 56 ++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/policy/policy_regression_test.go b/policy/policy_regression_test.go index 95f36f5..fae2fd4 100644 --- a/policy/policy_regression_test.go +++ b/policy/policy_regression_test.go @@ -199,3 +199,59 @@ func TestDecideReachesDenyOnlyAndIsOwnerWithNoStatements(t *testing.T) { } } } + +// IsAllowedActions reports self-service admin actions the way the server checks +// them -- implicitly granted, honoring only an explicit Deny -- while every +// other admin action needs an explicit Allow. The two halves of that split live +// on one line, so a swap between them stays invisible to policies that merely +// stopped carrying a Deny statement. +func TestIsAllowedActionsSelfServiceVsExplicitGrant(t *testing.T) { + tests := []struct { + name string + doc string + action AdminAction + want bool + }{ + { + // Nothing in the policy mentions it, so the DenyOnly path has to + // grant it anyway. + name: "self-service action implicit without an allow", + doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::b/*"]}]}`, + action: ChangeMyPasswordAdminAction, + want: true, + }, + { + // The one thing DenyOnly still respects. + name: "self-service action removed by an explicit deny", + doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Action":["admin:ChangeMyPassword"]}]}`, + action: ChangeMyPasswordAdminAction, + want: false, + }, + { + // A privileged action must never ride in on the DenyOnly path. + name: "privileged action absent without an allow", + doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::b/*"]}]}`, + action: CreateUserAdminAction, + want: false, + }, + { + name: "privileged action present with an explicit allow", + doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["admin:CreateUser"]}]}`, + action: CreateUserAdminAction, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p, err := ParseConfig(bytes.NewReader([]byte(tt.doc))) + if err != nil { + t.Fatal(err) + } + got := p.IsAllowedActions("", "", map[string][]string{}).Match(Action(tt.action)) + if got != tt.want { + t.Errorf("IsAllowedActions contains %s = %v, want %v", tt.action, got, tt.want) + } + }) + } +}