diff --git a/internal/daemon/controller/handlers/scopes/scope_service.go b/internal/daemon/controller/handlers/scopes/scope_service.go index 0c23f14c6b..69a7148ba9 100644 --- a/internal/daemon/controller/handlers/scopes/scope_service.go +++ b/internal/daemon/controller/handlers/scopes/scope_service.go @@ -22,6 +22,7 @@ import ( "github.com/hashicorp/boundary/internal/daemon/controller/handlers/sessions" "github.com/hashicorp/boundary/internal/daemon/controller/handlers/targets" "github.com/hashicorp/boundary/internal/daemon/controller/handlers/users" + "github.com/hashicorp/boundary/internal/daemon/controller/handlers/workers" "github.com/hashicorp/boundary/internal/errors" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" @@ -63,6 +64,7 @@ var ( resource.Role: roles.CollectionActions, resource.Scope: CollectionActions, resource.User: users.CollectionActions, + resource.Worker: workers.CollectionActions, }, scope.Org.String(): { diff --git a/internal/daemon/controller/handlers/scopes/scope_service_test.go b/internal/daemon/controller/handlers/scopes/scope_service_test.go index 989f45e9dd..0e1a82d379 100644 --- a/internal/daemon/controller/handlers/scopes/scope_service_test.go +++ b/internal/daemon/controller/handlers/scopes/scope_service_test.go @@ -95,6 +95,12 @@ var globalAuthorizedCollectionActions = map[string]*structpb.ListValue{ structpb.NewStringValue("list"), }, }, + "workers": { + Values: []*structpb.Value{ + structpb.NewStringValue("create:worker-request"), + structpb.NewStringValue("list"), + }, + }, } var orgAuthorizedCollectionActions = map[string]*structpb.ListValue{ diff --git a/internal/daemon/controller/handlers/workers/worker_service.go b/internal/daemon/controller/handlers/workers/worker_service.go index 372781051f..ef579122e0 100644 --- a/internal/daemon/controller/handlers/workers/worker_service.go +++ b/internal/daemon/controller/handlers/workers/worker_service.go @@ -36,7 +36,7 @@ var ( // CollectionActions contains the set of actions that can be performed on // this collection CollectionActions = action.ActionSet{ - action.Create, + action.CreateWorkerRequest, action.List, } ) diff --git a/internal/perms/acl.go b/internal/perms/acl.go index c4e813c1d6..d31b1838a5 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -1,18 +1,5 @@ package perms -/* -A really useful page to be aware of when looking at ACLs is -https://hashicorp.atlassian.net/wiki/spaces/ICU/pages/866976600/API+Actions+and+Permissions -speaking of which: TODO: put that chart in public docs. - -Anyways, from that page you can see that there are really only a few patterns of -ACLs that are ever allowed; see the Allowed function for a description along -with the code. - -This makes it actually quite simple to perform the ACL checking. Much of ACL -construction is thus synthesizing something reasonable from a set of Grants. -*/ - import ( "strings" @@ -114,6 +101,13 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { // If the action was not found above but we did find output fields in // patterns that match, we do not authorize the request, but we do build // up the output fields patterns. + // + // Note that when using IsActionOrParent it is merely to test whether it + // is an allowed format since some formats operate ony on collections + // (or don't operate at all on collections) and we want to ensure that + // it is/isn't a create or list command or subcommand to know whether + // that form is valid. The actual checking of whether the given action + // is granted to the user already happened above. var found bool switch { // id=;actions= where ID cannot be a wildcard; or @@ -123,8 +117,8 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { grant.id != "" && grant.id != "*" && grant.typ == resource.Unknown && - aType != action.List && - aType != action.Create: + !action.List.IsActionOrParent(aType) && + !action.Create.IsActionOrParent(aType): found = true @@ -137,8 +131,8 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { grant.typ == r.Type && grant.typ != resource.Unknown && topLevelType(r.Type) && - (aType == action.List || - aType == action.Create): + (action.List.IsActionOrParent(aType) || + action.Create.IsActionOrParent(aType)): found = true @@ -187,7 +181,8 @@ func topLevelType(typ resource.Type) bool { resource.Scope, resource.Session, resource.Target, - resource.User: + resource.User, + resource.Worker: return true } return false diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index c14edd65af..d32845112a 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" + "github.com/hashicorp/boundary/internal/types/scope" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -315,6 +316,36 @@ func Test_ACLAllowed(t *testing.T) { {action: action.ReadSelf, authorized: true}, }, }, + { + name: "create worker with create", + resource: Resource{ScopeId: scope.Global.String(), Type: resource.Worker}, + scopeGrants: []scopeGrant{ + { + scope: scope.Global.String(), + grants: []string{ + "type=worker;actions=create", + }, + }, + }, + actionsAuthorized: []actionAuthorized{ + {action: action.CreateWorkerRequest, authorized: true}, + }, + }, + { + name: "create worker with request only", + resource: Resource{ScopeId: scope.Global.String(), Type: resource.Worker}, + scopeGrants: []scopeGrant{ + { + scope: scope.Global.String(), + grants: []string{ + "type=worker;actions=create:worker-request", + }, + }, + }, + actionsAuthorized: []actionAuthorized{ + {action: action.CreateWorkerRequest, authorized: true}, + }, + }, } for _, test := range tests { diff --git a/internal/perms/grants.go b/internal/perms/grants.go index 405f7de2ba..279573f388 100644 --- a/internal/perms/grants.go +++ b/internal/perms/grants.go @@ -74,6 +74,20 @@ func (g Grant) Actions() (typs []action.Type, strs []string) { return } +// hasActionOrSubaction checks whether a grant's action set contains the given +// action or contains an action that is a subaction of the passed-in parameter. +// This is used for validation checking of parsed grants. N.B.: this is the +// opposite check of action.Type.IsActionOrParent, which is why the ordering is +// reversed going into that call. +func (g Grant) hasActionOrSubaction(act action.Type) bool { + for k := range g.actions { + if act.IsActionOrParent(k) { + return true + } + } + return false +} + func (g Grant) clone() *Grant { ret := &Grant{ scope: g.scope, @@ -376,12 +390,12 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { return Grant{}, errors.NewDeprecated(errors.InvalidParameter, op, "parsed grant string contains no actions or output fields") } case 1: - if !grant.actions[action.Create] && - !grant.actions[action.List] { + if !grant.hasActionOrSubaction(action.Create) && + !grant.hasActionOrSubaction(action.List) { return Grant{}, errors.NewDeprecated(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] { + if !grant.hasActionOrSubaction(action.Create) || !grant.hasActionOrSubaction(action.List) { return Grant{}, errors.NewDeprecated(errors.InvalidParameter, op, "parsed grant string contains non-create or non-list action in a format that only allows these") } default: @@ -411,6 +425,7 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { results := acl.Allowed(r, k) if results.Authorized { allowed = true + break } } if !allowed { @@ -429,7 +444,7 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { func (g Grant) validateType() error { const op = "perms.(Grant).validateType" switch g.typ { - case resource.Controller, resource.Worker: + case resource.Controller: return errors.NewDeprecated(errors.InvalidParameter, op, fmt.Sprintf("unknown type specifier %q", g.typ)) } return nil diff --git a/internal/perms/grants_test.go b/internal/perms/grants_test.go index 19f8072804..fd4c950c0e 100644 --- a/internal/perms/grants_test.go +++ b/internal/perms/grants_test.go @@ -110,7 +110,7 @@ func Test_ValidateType(t *testing.T) { var g Grant for i := resource.Unknown; i <= resource.CredentialLibrary; i++ { g.typ = i - if i == resource.Controller || i == resource.Worker { + if i == resource.Controller { assert.Error(t, g.validateType()) } else { assert.NoError(t, g.validateType()) @@ -705,3 +705,52 @@ func Test_Parse(t *testing.T) { }) } } + +func TestHasActionOrSubaction(t *testing.T) { + tests := []struct { + name string + base []action.Type + act action.Type + want bool + }{ + { + name: "no actions", + base: []action.Type{}, + act: action.Read, + }, + { + name: "has direct action", + base: []action.Type{action.Cancel, action.Read}, + act: action.Read, + want: true, + }, + { + name: "has parent action", + base: []action.Type{action.Cancel, action.ReadSelf}, + act: action.Read, + want: true, + }, + { + name: "has direct sub action", + base: []action.Type{action.Cancel, action.ReadSelf}, + act: action.ReadSelf, + want: true, + }, + { + name: "has sub action needs parent", + base: []action.Type{action.Cancel, action.Read}, + act: action.ReadSelf, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := Grant{ + actions: make(map[action.Type]bool), + } + for _, act := range tt.base { + g.actions[act] = true + } + assert.Equal(t, tt.want, g.hasActionOrSubaction(tt.act)) + }) + } +} diff --git a/internal/types/action/action.go b/internal/types/action/action.go index e7d00aa7d0..dec8cfc43c 100644 --- a/internal/types/action/action.go +++ b/internal/types/action/action.go @@ -1,12 +1,12 @@ package action -import "strings" +import ( + "fmt" + "strings" +) -// Type defines a type for the Actions of Resources -// actions are also stored as a lookup db table named iam_action type Type uint -// not using iota intentionally, since the values are stored in the db as well. const ( Unknown Type = 0 List Type = 1 @@ -53,6 +53,7 @@ const ( AddHostSources Type = 42 SetHostSources Type = 43 RemoveHostSources Type = 44 + CreateWorkerRequest Type = 45 ) var Map = map[string]Type{ @@ -100,6 +101,7 @@ var Map = map[string]Type{ AddHostSources.String(): AddHostSources, SetHostSources.String(): SetHostSources, RemoveHostSources.String(): RemoveHostSources, + CreateWorkerRequest.String(): CreateWorkerRequest, } func (a Type) String() string { @@ -149,9 +151,20 @@ func (a Type) String() string { "add-host-sources", "set-host-sources", "remove-host-sources", + "create:worker-request", }[a] } +// IsActionOrParent tests whether the given action is either the same as the +// suspect or is a parent of the suspect. This is used in some parts of ACL +// checking. +func (act Type) IsActionOrParent(suspect Type) bool { + if act == suspect { + return true + } + return strings.HasPrefix(suspect.String(), fmt.Sprintf("%s:", act.String())) +} + // ActionSet stores a slice of action types type ActionSet []Type diff --git a/internal/types/action/action_test.go b/internal/types/action/action_test.go index c9de615721..c5086248ee 100644 --- a/internal/types/action/action_test.go +++ b/internal/types/action/action_test.go @@ -95,6 +95,10 @@ func TestAction(t *testing.T) { action: NoOp, want: "no-op", }, + { + action: CreateWorkerRequest, + want: "create:worker-request", + }, } for _, tt := range tests { t.Run(tt.want, func(t *testing.T) { @@ -216,3 +220,40 @@ func TestOnlySelf(t *testing.T) { }) } } + +func TestIsActionOrParent(t *testing.T) { + tests := []struct { + name string + base Type + comp Type + want bool + }{ + { + name: "same", + base: Cancel, + comp: Cancel, + want: true, + }, + { + name: "different", + base: Cancel, + comp: Read, + }, + { + name: "different base and comp", + base: List, + comp: CancelSelf, + }, + { + name: "same base and comp", + base: Cancel, + comp: CancelSelf, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.base.IsActionOrParent(tt.comp)) + }) + } +} diff --git a/internal/types/resource/resource.go b/internal/types/resource/resource.go index 45edab66f5..c8f40e9cca 100644 --- a/internal/types/resource/resource.go +++ b/internal/types/resource/resource.go @@ -28,7 +28,7 @@ const ( // NOTE: When adding a new type, be sure to update: // // * The Grant.validateType function and test - // * The perms.topLevelTypes function + // * The perms.topLevelType function // * The scopes service collection actions for appropriate scopes )