Add worker creation ACLs (#2138)

This is a bit more than simply adding the action; we had some
checks to ensure that list/create could only happen/not happen
in some cases and those had to be modified to account for
subactions.

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(): {

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{

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,
 	}
 )

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=<resource.id>;actions=<action> 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

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 {

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

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))
+		})
+	}
+}

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
 

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))
+		})
+	}
+}

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
 )