From 1afa3a4b6c7c4fdcc0b8ddd4fe96543134328c59 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Fri, 5 Feb 2021 12:39:55 -0500 Subject: [PATCH] Disallow an invalid grant format (#914) --- CHANGELOG.md | 13 ++++ internal/auth/additional_verification_test.go | 2 +- internal/auth/auth.go | 3 + internal/iam/role_grant_test.go | 8 +-- internal/perms/acl.go | 4 +- internal/perms/acl_test.go | 12 ++-- internal/perms/grants.go | 50 ++++++++++++++- internal/perms/grants_test.go | 63 +++++++++++++++++-- .../docs/concepts/security/permissions.mdx | 5 +- 9 files changed, 138 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e73cd0b8..952921dc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. ## Next +### Changes/Deprecations + +* permissions: Update some errors to make them more descriptive, and disallow + permissions in some forms where they will never take effect, preventing + possible confusion (existing grants already saved to the database will not be + affected as this is only filtered when grants are added/set on a role): + * `id=;actions=` where one of the actions is `create` + or `list`. By definition this format operates only on individual resources + so `create` and `list` will never work + * `type=;actions=` where one of the actions is _not_ + `create` or `list`. This format operates only on collections so assigning + more actions this way will never work + ### New and Improved * server: When running single-server mode and `controllers` is not specified in diff --git a/internal/auth/additional_verification_test.go b/internal/auth/additional_verification_test.go index bb2fa2c6a..9c7a8a67b 100644 --- a/internal/auth/additional_verification_test.go +++ b/internal/auth/additional_verification_test.go @@ -39,7 +39,7 @@ func TestFetchActionSetForId(t *testing.T) { orgRole := iam.TestRole(t, conn, org.GetPublicId()) iam.TestUserRole(t, conn, orgRole.PublicId, token.UserId) iam.TestRoleGrant(t, conn, orgRole.PublicId, "id=foo;actions=read,update") - iam.TestRoleGrant(t, conn, orgRole.PublicId, "id=bar;actions=read,update,delete,list,authorize-session") + iam.TestRoleGrant(t, conn, orgRole.PublicId, "id=bar;actions=read,update,delete,authorize-session") iam.TestRoleGrant(t, conn, orgRole.PublicId, "id=*;type=role;actions=add-grants,remove-grants") cases := []struct { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index aa27c2dbe..a0a5d318b 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -422,6 +422,9 @@ func (v verifier) performAuthCheck() (aclResults perms.ACLResults, userId string return } parsedGrants = make([]perms.Grant, 0, len(grantPairs)) + // Note: Below, we always skip validation so that we don't error on formats + // that we've since restricted, e.g. "id=foo;actions=create,read". These + // will simply not have an effect. for _, pair := range grantPairs { parsed, err := perms.Parse( pair.ScopeId, diff --git a/internal/iam/role_grant_test.go b/internal/iam/role_grant_test.go index c67f01ca2..ca8bbddc2 100644 --- a/internal/iam/role_grant_test.go +++ b/internal/iam/role_grant_test.go @@ -178,14 +178,14 @@ func TestRoleGrant_Delete(t *testing.T) { name: "invalid", args: args{ roleId: projRole.PublicId, - grant: "id=a_edcb;actions=create,update", + grant: "id=a_edcb;actions=read,update", }, }, { name: "valid", args: args{ roleId: projRole.PublicId, - grant: "id=a_bcde;actions=create,update", + grant: "id=a_bcde;actions=read,update", }, deletedRows: 1, }, @@ -194,10 +194,10 @@ func TestRoleGrant_Delete(t *testing.T) { t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) require.NoError(conn.Where("1=1").Delete(allocRoleGrant()).Error) - rg, err := NewRoleGrant(projRole.PublicId, "id=a_bcde;actions=create,update") + rg, err := NewRoleGrant(projRole.PublicId, "id=a_bcde;actions=read,update") require.NoError(err) require.NoError(rw.Create(context.Background(), rg)) - rg, err = NewRoleGrant(projRole.PublicId, "id=a_cdef;actions=create,update") + rg, err = NewRoleGrant(projRole.PublicId, "id=a_cdef;actions=read,update") require.NoError(err) require.NoError(rw.Create(context.Background(), rg)) diff --git a/internal/perms/acl.go b/internal/perms/acl.go index da9f74efb..bee2805df 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -98,7 +98,9 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { case grant.id == r.Id && grant.id != "" && grant.id != "*" && - grant.typ == resource.Unknown: + grant.typ == resource.Unknown && + aType != action.List && + aType != action.Create: results.Allowed = true return diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index a416b784b..8573f3ff1 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -60,7 +60,7 @@ func Test_ACLAllowed(t *testing.T) { scope: "o_d", grants: []string{ "id=*;type=*;actions=create,update", - "type=session;actions=list:self", + "id=*;type=session;actions=*", }, }, } @@ -255,21 +255,19 @@ func Test_ACLAllowed(t *testing.T) { userId: "u_abcd1234", }, { - name: "list self with top level list", + name: "list with top level list", resource: Resource{ScopeId: "o_a", Type: resource.Target}, scopeGrants: commonGrants, actionsAllowed: []actionAllowed{ {action: action.List, allowed: true}, - {action: action.ListSelf, allowed: true}, }, }, { - name: "list self with top level list self", + name: "list sessions with wildcard actions", resource: Resource{ScopeId: "o_d", Type: resource.Session}, scopeGrants: commonGrants, actionsAllowed: []actionAllowed{ - {action: action.List}, - {action: action.ListSelf, allowed: true}, + {action: action.List, allowed: true}, }, }, { @@ -304,7 +302,7 @@ func Test_ACLAllowed(t *testing.T) { } acl := NewACL(grants...) for _, aa := range test.actionsAllowed { - assert.True(t, acl.Allowed(test.resource, aa.action).Allowed == aa.allowed) + assert.True(t, acl.Allowed(test.resource, aa.action).Allowed == aa.allowed, "action: %s, acl allowed: %t, test action allowed: %t", aa.action, acl.Allowed(test.resource, aa.action).Allowed, aa.allowed) } }) } diff --git a/internal/perms/grants.go b/internal/perms/grants.go index 6b7809a0a..7b98b734e 100644 --- a/internal/perms/grants.go +++ b/internal/perms/grants.go @@ -302,8 +302,54 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { } if !opts.withSkipFinalValidation { - // Validate the grant. Create a dummy resource and pass it through - // Allowed and ensure that we get allowed. + // Filter out some forms that don't make sense + + // First up, an ID is given, no type, and actions contains "create" or + // "list". Note wildcard for actions is still okay. + if grant.id != "" && grant.typ == resource.Unknown { + if grant.actions[action.Create] || + grant.actions[action.List] { + return Grant{}, errors.New(errors.InvalidParameter, op, "parsed grant string contains create or list action in a format that does not allow these") + } + } + // If no ID is given... + if grant.id == "" { + // Check the type + switch grant.typ { + case resource.Unknown: + // Error -- no ID or type isn't valid (although we should never + // get to this point because original parsing should error) + return Grant{}, errors.New(errors.InvalidParameter, op, "parsed grant string contains no id or type") + case resource.All: + // "type=*;actions=..." is not supported -- we reqiure you to + // explicitly set a pin or set the ID to * + return Grant{}, errors.New(errors.InvalidParameter, op, "parsed grant string contains wildcard type with no id value") + default: + // Here we have type=something,actions=. This + // means we're operating on collections. Note that wildcard + // actions are not okay here; that uses the format + // id=*;type=;actions=* + switch len(grant.actions) { + case 0: + // A total lack of actions is already caught elsewhere but + // this is here for completeness + return Grant{}, errors.New(errors.InvalidParameter, op, "parsed grant string contains no actions") + case 1: + if !grant.actions[action.Create] && + !grant.actions[action.List] { + return Grant{}, errors.New(errors.InvalidParameter, op, "parsed grant string contains non-create or non-list action in a format that only allows these") + } + case 2: + if !grant.actions[action.Create] || !grant.actions[action.List] { + return Grant{}, errors.New(errors.InvalidParameter, op, "parsed grant string contains non-create or non-list action in a format that only allows these") + } + default: + return Grant{}, errors.New(errors.InvalidParameter, op, "parsed grant string contains non-create or non-list action in a format that only allows these") + } + } + } + // Create a dummy resource and pass it through Allowed and ensure that + // we get allowed. acl := NewACL(grant) r := Resource{ ScopeId: scopeId, diff --git a/internal/perms/grants_test.go b/internal/perms/grants_test.go index 91e22b91b..2d9a6aeec 100644 --- a/internal/perms/grants_test.go +++ b/internal/perms/grants_test.go @@ -347,10 +347,61 @@ func Test_Parse(t *testing.T) { input: "id=foobar;type=host-catalog;actions=createread", err: `perms.Parse: perms.(Grant).parseAndValidateActions: unknown action "createread": parameter violation: error #100`, }, + { + name: "bad create action for id", + input: "id=foobar;actions=create", + err: `perms.Parse: parsed grant string contains create or list action in a format that does not allow these: parameter violation: error #100`, + }, + { + name: "bad create action for id with other perms", + input: "id=foobar;actions=read,create", + err: `perms.Parse: parsed grant string contains create or list action in a format that does not allow these: parameter violation: error #100`, + }, + { + name: "bad list action for id", + input: "id=foobar;actions=list", + err: `perms.Parse: parsed grant string contains create or list action in a format that does not allow these: parameter violation: error #100`, + }, + { + name: "bad list action for id with other perms", + input: "type=host-catalog;actions=list,read", + err: `perms.Parse: parsed grant string contains non-create or non-list action in a format that only allows these: parameter violation: error #100`, + }, + { + name: "wildcard id and actions without collection", + input: "id=*;actions=read", + err: `perms.Parse: parsed grant string would not result in any action being authorized: parameter violation: error #100`, + }, + { + name: "wildcard id and actions with list", + input: "id=*;actions=read,list", + err: `perms.Parse: parsed grant string contains create or list action in a format that does not allow these: parameter violation: error #100`, + }, + { + name: "wildcard type with no id", + input: "type=*;actions=read,list", + err: `perms.Parse: parsed grant string contains wildcard type with no id value: parameter violation: error #100`, + }, { name: "empty id and type", input: "actions=create", - err: `perms.Parse: parsed grant string would not result in any action being authorized: parameter violation: error #100`, + err: `perms.Parse: parsed grant string contains no id or type: parameter violation: error #100`, + }, + { + name: "wildcard id and type and actions with list", + input: "id=*;type=*;actions=read,list", + expected: Grant{ + scope: Scope{ + Id: "o_scope", + Type: scope.Org, + }, + id: "*", + typ: resource.All, + actions: map[action.Type]bool{ + action.Read: true, + action.List: true, + }, + }, }, { name: "good json type", @@ -466,7 +517,7 @@ func Test_Parse(t *testing.T) { }, { name: "good user id template", - input: `id={{ user.id}};actions=create,read`, + input: `id={{ user.id}};actions=read,update`, userId: "u_abcd1234", expected: Grant{ scope: Scope{ @@ -475,20 +526,20 @@ func Test_Parse(t *testing.T) { }, id: "u_abcd1234", actions: map[action.Type]bool{ - action.Create: true, + action.Update: true, action.Read: true, }, }, }, { name: "bad account id template", - input: `id={{superman}};actions=create,read`, + input: `id={{superman}};actions=read`, accountId: "apw_1234567890", err: `perms.Parse: unknown template "{{superman}}" in grant "id" value: parameter violation: error #100`, }, { name: "good account id template", - input: `id={{ account.id}};actions=create,read`, + input: `id={{ account.id}};actions=update,read`, accountId: "apw_1234567890", expected: Grant{ scope: Scope{ @@ -497,7 +548,7 @@ func Test_Parse(t *testing.T) { }, id: "apw_1234567890", actions: map[action.Type]bool{ - action.Create: true, + action.Update: true, action.Read: true, }, }, diff --git a/website/content/docs/concepts/security/permissions.mdx b/website/content/docs/concepts/security/permissions.mdx index 6078edb38..6fcc291b3 100644 --- a/website/content/docs/concepts/security/permissions.mdx +++ b/website/content/docs/concepts/security/permissions.mdx @@ -76,7 +76,10 @@ Example: `id=hsst_1234567890;actions=read,update` -This grants `read` and `update` actions to that single resource. +This grants `read` and `update` actions to that single resource. It is invalid +to specify `create` or `list` as actions in this format, as this format +explicitly identifies a resource, whereas those actions operate exclusively on +collections. ### Type Only