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..fae2fd4 100644 --- a/policy/policy_regression_test.go +++ b/policy/policy_regression_test.go @@ -68,45 +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()), - }, + stmt := func(effect Effect, action Action) Statement { + return NewStatement("", effect, NewActionSet(action), + NewResourceSet(NewResource("*")), condition.NewFunctions()) } - if !literal.HasDenyStatement() { - t.Error("struct literal policy carries a Deny but HasDenyStatement() reports false") + + 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, + }, + { + // 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, + }, } - 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) - } + 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) } - } - } - if checked == 0 { - t.Error("none of the named canned policies carries a Deny; this test checked nothing") + }) } } @@ -191,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) + } + }) + } +}