Disallow an invalid grant format (#914)

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=<some_id>;actions=<some_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=<some_type>;actions=<some_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

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 {

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,

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

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

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

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=<something else>. This
+				// means we're operating on collections. Note that wildcard
+				// actions are not okay here; that uses the format
+				// id=*;type=<something>;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,

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

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