Restrict anonymous user capabilities (#2156)

CHANGELOG.md

@@ -4,6 +4,17 @@ Canonical reference for changes, improvements, and bugfixes for Boundary.
 
 ## Next
 
+### Deprecations/Changes
+
+* Anonymous user permissions: In order to reduce the risk of accidental and
+  unintended granting of permissions to anonymous users, the permissions system
+  now only allows certain actions on certain resources to be assigned to the
+  anonymous user; currently these are the same permissions as assigned in
+  Boundary's default role permissions. If other use-cases arise this list can be
+  expanded. See [the
+  documentation](https://www.boundaryproject.io/docs/concepts/security/permissions/assignable-permissions)
+  for more details.
+
 ### Bug Fixes
 
 ## 0.8.1 (2022/05/13)
@@ -13,7 +24,8 @@ Canonical reference for changes, improvements, and bugfixes for Boundary.
 * controller: Do not shut down cluster listener when it receives an invalid
   packet ([Issue](https://github.com/hashicorp/boundary/issues/2072),
   [PR](https://github.com/hashicorp/boundary/pull/2073))
-* session: update cancel_session() function to check for terminated state ([Issue](https://github.com/hashicorp/boundary/issues/2064),
+* session: update cancel_session() function to check for terminated state
+  ([Issue](https://github.com/hashicorp/boundary/issues/2064),
   [PR](https://github.com/hashicorp/boundary/pull/2065))
 
 ## 0.8.0 (2022/05/03)
@@ -29,23 +41,26 @@ Canonical reference for changes, improvements, and bugfixes for Boundary.
 
 ### Bug Fixes
 
-* worker: create new error to prevent `event.newError: missing error: invalid parameter` and handle session cancel 
-  with no TOFU token ([Issue](https://github.com/hashicorp/boundary/issues/1902),
+* worker: create new error to prevent `event.newError: missing error: invalid
+  parameter` and handle session cancel with no TOFU token
+  ([Issue](https://github.com/hashicorp/boundary/issues/1902),
   [PR](https://github.com/hashicorp/boundary/pull/1929))
-* controller: Reconcile DEKs with existing scopes ([Issue](https://github.com/hashicorp/boundary/issues/1856),
+* controller: Reconcile DEKs with existing scopes
+  ([Issue](https://github.com/hashicorp/boundary/issues/1856),
   [PR](https://github.com/hashicorp/boundary/pull/1976))
-* controller: Fix for retrieving sessions that could result in incomplete results when
-  there is a large number (10k+) of sessions.
+* controller: Fix for retrieving sessions that could result in incomplete
+  results when there is a large number (10k+) of sessions.
   [PR](https://github.com/hashicorp/boundary/pull/2049)
-* session: update session state trigger to prevent transitions to invalid states ([Issue](https://github.com/hashicorp/boundary/issues/2040),
+* session: update session state trigger to prevent transitions to invalid states
+  ([Issue](https://github.com/hashicorp/boundary/issues/2040),
   [PR](https://github.com/hashicorp/boundary/pull/2046))
 
 ## 0.7.6 (2022/03/15)
 
 ### Bug Fixes
 
-* sessions: Sessions and session connections have been refactored 
-to better isolate transactions and prevent resource contention that caused deadlocks.
+* sessions: Sessions and session connections have been refactored to better
+isolate transactions and prevent resource contention that caused deadlocks.
 ([Issue](https://github.com/hashicorp/boundary/issues/1812),
   [PR](https://github.com/hashicorp/boundary/pull/1919))
 * scheduler: Fix bug that causes erroneous logs when racing controllers

internal/daemon/controller/auth/auth.go

@@ -569,7 +569,7 @@ func (v verifier) performAuthCheck(ctx context.Context) (
 	}
 
 	retAcl = perms.NewACL(parsedGrants...)
-	aclResults = retAcl.Allowed(*v.res, v.act)
+	aclResults = retAcl.Allowed(*v.res, v.act, userId)
 	// We don't set authenticated above because setting this but not authorized
 	// is used for further permissions checks, such as during recursive listing.
 	// So we want to make sure any code relying on that has the full set of
@@ -619,7 +619,7 @@ func (r *VerifyResults) fetchActions(id string, typ resource.Type, availableActi
 
 	ret := make(action.ActionSet, 0, len(availableActions))
 	for _, act := range availableActions {
-		if r.v.acl.Allowed(*res, act).Authorized {
+		if r.v.acl.Allowed(*res, act, r.UserId).Authorized {
 			ret = append(ret, act)
 		}
 	}
@@ -637,7 +637,7 @@ func (r *VerifyResults) FetchOutputFields(res perms.Resource, act action.Type) p
 		return nil
 	}
 
-	return r.v.acl.Allowed(res, act).OutputFields
+	return r.v.acl.Allowed(res, act, r.UserId).OutputFields
 }
 
 // GetTokenFromRequest pulls the token from either the Authorization header or
@@ -675,7 +675,7 @@ func GetTokenFromRequest(ctx context.Context, kmsCache *kms.Kms, req *http.Reque
 	if receivedTokenType == AuthTokenTypeUnknown || fullToken == "" {
 		// We didn't find auth info or a client screwed up and put in a blank
 		// header instead of nothing at all, so return blank which will indicate
-		// the anonymouse user
+		// the anonymous user
 		return "", "", uint32(AuthTokenTypeUnknown)
 	}
 

internal/perms/acl.go

@@ -41,7 +41,7 @@ type ACLResults struct {
 }
 
 // Resource defines something within boundary that requires authorization
-// capabilities.  Resources must have a ScopeId.
+// capabilities. Resources must have a ScopeId.
 type Resource struct {
 	// ScopeId is the scope that contains the Resource.
 	ScopeId string `json:"scope_id,omitempty"`
@@ -71,7 +71,9 @@ func NewACL(grants ...Grant) ACL {
 }
 
 // Allowed determines if the grants for an ACL allow an action for a resource.
-func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) {
+func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option) (results ACLResults) {
+	opts := getOpts(opt...)
+
 	// First, get the grants within the specified scope
 	grants := a.scopeMap[r.ScopeId]
 	results.scopeMap = a.scopeMap
@@ -116,6 +118,30 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) {
 		// up the output fields patterns.
 		var found bool
 		switch {
+		// Case 1: We only allow specific actions on specific types for the
+		// anonymous user. ID being supplied or not doesn't matter in this case,
+		// it must be an explicit type and action(s); adding this as an explicit
+		// case here prevents duplicating logic in two of the other more
+		// general-purpose cases below (3 and 4). See notes there about ID being
+		// present or not.
+		case !opts.withSkipAnonymousUserRestrictions &&
+			(userId == AnonymousUserId || userId == ""):
+			switch {
+			// Allow discovery of scopes, so that auth methods within can be
+			// discovered
+			case grant.typ == r.Type &&
+				grant.typ == resource.Scope &&
+				(aType == action.List || aType == action.NoOp):
+				found = true
+
+			// Allow discovery of and authenticating to auth methods
+			case grant.typ == r.Type &&
+				grant.typ == resource.AuthMethod &&
+				(aType == action.List || aType == action.NoOp || aType == action.Authenticate):
+				found = true
+			}
+
+		// Case 2:
 		// id=<resource.id>;actions=<action> where ID cannot be a wildcard; or
 		// id=<resource.id>;output_fields=<fields> where fields cannot be a
 		// wildcard.
@@ -128,10 +154,21 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) {
 
 			found = true
 
-		// type=<resource.type>;actions=<action> when action is list or create.
-		// Must be a top level collection, otherwise must be one of the two
-		// formats specified below. Or,
-		// type=resource.type;output_fields=<fields> and no action.
+		// Case 3: type=<resource.type>;actions=<action> when action is list or
+		// create. Must be a top level collection, otherwise must be one of the
+		// two formats specified in cases 4 or 5. Or,
+		// type=resource.type;output_fields=<fields> and no action. This is more
+		// of a semantic difference compared to 4 more than a security
+		// difference; this type is for clarity as it ties more closely to the
+		// concept of create and list as actions on a collection, operating on a
+		// collection directly. The format in case 4 will still work for
+		// create/list on collections but that's more of a shortcut to allow
+		// things like id=*;type=*;actions=* for admin flows so that you don't
+		// need to separate out explicit collection actions into separate typed
+		// grants for each collection within a role. This does mean there are
+		// "two ways of doing things" but it's a reasonable UX tradeoff given
+		// that "all IDs" can reasonably be construed to include "and the one
+		// I'm making" and "all of them for listing".
 		case grant.id == "" &&
 			r.Id == "" &&
 			grant.typ == r.Type &&
@@ -142,6 +179,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) {
 
 			found = true
 
+		// Case 4:
 		// id=*;type=<resource.type>;actions=<action> where type cannot be
 		// unknown but can be a wildcard to allow any resource at all; or
 		// id=*;type=<resource.type>;output_fields=<fields> with no action.
@@ -152,6 +190,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) {
 
 			found = true
 
+		// Case 5:
 		// id=<pin>;type=<resource.type>;actions=<action> where type can be a
 		// wildcard and this this is operating on a non-top-level type. Same for
 		// output fields only.

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"
 )
@@ -329,8 +330,12 @@ func Test_ACLAllowed(t *testing.T) {
 			}
 			acl := NewACL(grants...)
 			for _, aa := range test.actionsAuthorized {
-				result := acl.Allowed(test.resource, aa.action)
-				assert.True(t, result.Authorized == aa.authorized, "action: %s, acl authorized: %t, test action authorized: %t", aa.action, acl.Allowed(test.resource, aa.action).Authorized, aa.authorized)
+				userId := test.userId
+				if userId == "" {
+					userId = "u_1234567890"
+				}
+				result := acl.Allowed(test.resource, aa.action, userId)
+				assert.True(t, result.Authorized == aa.authorized, "action: %s, acl authorized: %t, test action authorized: %t", aa.action, result.Authorized, aa.authorized)
 				assert.ElementsMatch(t, result.OutputFields.Fields(), aa.outputFields)
 			}
 		})
@@ -349,3 +354,85 @@ func TestJsonMarshal(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, `{"scope_id":"scope","id":"id","type":"controller"}`, string(out))
 }
+
+// Test_AnonRestrictions loops through every resource and action and ensures
+// that it always fails for the anonymous user, regardless of what is granted,
+// except for the specific things allowed.
+func Test_AnonRestrictions(t *testing.T) {
+	t.Parallel()
+
+	type input struct {
+		name              string
+		grant             string
+		templatedType     bool
+		shouldHaveSuccess bool
+	}
+	tests := []input{
+		{
+			name:  "id-specific",
+			grant: "id=foobar;actions=%s",
+		},
+		{
+			name:              "wildcard-id",
+			grant:             "id=*;type=%s;actions=%s",
+			templatedType:     true,
+			shouldHaveSuccess: true,
+		},
+		{
+			name:  "wildcard-id-and-type",
+			grant: "id=*;type=*;actions=%s",
+		},
+		{
+			name:              "no-id",
+			grant:             "type=%s;actions=%s",
+			templatedType:     true,
+			shouldHaveSuccess: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			require, assert := require.New(t), assert.New(t)
+			for i := resource.Type(1); i <= resource.CredentialLibrary; i++ {
+				if i == resource.Controller || i == resource.Worker {
+					continue
+				}
+				for j := action.Type(1); j <= action.RemoveHostSources; j++ {
+					res := Resource{
+						ScopeId: scope.Global.String(),
+						Id:      "foobar",
+						Type:    resource.Type(i),
+					}
+					grant := test.grant
+					if test.templatedType {
+						grant = fmt.Sprintf(grant, resource.Type(i).String(), action.Type(j).String())
+					} else {
+						grant = fmt.Sprintf(grant, action.Type(j).String())
+					}
+
+					parsedGrant, err := Parse(scope.Global.String(), grant, WithSkipFinalValidation(true))
+					require.NoError(err)
+
+					acl := NewACL(parsedGrant)
+					results := acl.Allowed(res, action.Type(j), AnonymousUserId)
+
+					switch test.shouldHaveSuccess {
+					case true:
+						// Ensure it's one of the specific cases and fail otherwise
+						switch {
+						case i == resource.Scope && (j == action.List || j == action.NoOp):
+							assert.True(results.Authorized, fmt.Sprintf("i: %v, j: %v", i, j))
+						case i == resource.AuthMethod && (j == action.List || j == action.NoOp || j == action.Authenticate):
+							assert.True(results.Authorized, fmt.Sprintf("i: %v, j: %v", i, j))
+						default:
+							assert.False(results.Authorized, fmt.Sprintf("i: %v, j: %v", i, j))
+						}
+					default:
+						// Should always fail
+						assert.False(results.Authorized)
+					}
+				}
+			}
+		})
+	}
+}

internal/perms/grants.go

@@ -408,7 +408,7 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) {
 			}
 			var allowed bool
 			for k := range grant.actions {
-				results := acl.Allowed(r, k)
+				results := acl.Allowed(r, k, AnonymousUserId, WithSkipAnonymousUserRestrictions(true))
 				if results.Authorized {
 					allowed = true
 				}

internal/perms/option.go

@@ -4,7 +4,9 @@ package perms
 func getOpts(opt ...Option) options {
 	opts := getDefaultOptions()
 	for _, o := range opt {
-		o(&opts)
+		if o != nil {
+			o(&opts)
+		}
 	}
 	return opts
 }
@@ -14,9 +16,10 @@ type Option func(*options)
 
 // options = how options are represented
 type options struct {
-	withUserId              string
-	withAccountId           string
-	withSkipFinalValidation bool
+	withUserId                        string
+	withAccountId                     string
+	withSkipFinalValidation           bool
+	withSkipAnonymousUserRestrictions bool
 }
 
 func getDefaultOptions() options {
@@ -45,3 +48,12 @@ func WithSkipFinalValidation(skipFinalValidation bool) Option {
 		o.withSkipFinalValidation = skipFinalValidation
 	}
 }
+
+// WithSkipAnonymousUserRestrictions allows skipping the restrictions on
+// anonymous users, useful when e.g. validating parsed grants where we may not
+// have a user ID yet.
+func WithSkipAnonymousUserRestrictions(with bool) Option {
+	return func(o *options) {
+		o.withSkipAnonymousUserRestrictions = with
+	}
+}

internal/perms/option_test.go

@@ -0,0 +1,43 @@
+package perms
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_GetOpts(t *testing.T) {
+	t.Parallel()
+	t.Run("nil", func(t *testing.T) {
+		opts := getOpts(nil)
+		assert.NotNil(t, opts)
+	})
+	t.Run("with-user-id", func(t *testing.T) {
+		assert := assert.New(t)
+		opts := getOpts()
+		assert.Empty(opts.withUserId)
+		opts = getOpts(WithUserId("foo"))
+		assert.Equal("foo", opts.withUserId)
+	})
+	t.Run("with-account-id", func(t *testing.T) {
+		assert := assert.New(t)
+		opts := getOpts()
+		assert.Empty(opts.withAccountId)
+		opts = getOpts(WithAccountId("foo"))
+		assert.Equal("foo", opts.withAccountId)
+	})
+	t.Run("with-skip-final-validation", func(t *testing.T) {
+		assert := assert.New(t)
+		opts := getOpts()
+		assert.False(opts.withSkipFinalValidation)
+		opts = getOpts(WithSkipFinalValidation(true))
+		assert.True(opts.withSkipFinalValidation)
+	})
+	t.Run("with-skip-anonymous-user-restrictions", func(t *testing.T) {
+		assert := assert.New(t)
+		opts := getOpts()
+		assert.False(opts.withSkipAnonymousUserRestrictions)
+		opts = getOpts(WithSkipAnonymousUserRestrictions(true))
+		assert.True(opts.withSkipAnonymousUserRestrictions)
+	})
+}

internal/perms/output_fields_test.go

@@ -221,7 +221,7 @@ func Test_ACLOutputFields(t *testing.T) {
 				grants = append(grants, grant)
 			}
 			acl := NewACL(grants...)
-			results := acl.Allowed(test.resource, test.action)
+			results := acl.Allowed(test.resource, test.action, AnonymousUserId, WithSkipAnonymousUserRestrictions(true))
 			assert.ElementsMatch(t, results.OutputFields.Fields(), test.fields)
 			assert.True(t, test.authorized == results.Authorized)
 		})

internal/types/action/action.go

@@ -53,6 +53,10 @@ const (
 	AddHostSources            Type = 42
 	SetHostSources            Type = 43
 	RemoveHostSources         Type = 44
+
+	// When adding new actions, be sure to update:
+	//
+	// * The Test_AnonRestrictions test
 )
 
 var Map = map[string]Type{

internal/types/resource/resource.go

@@ -30,6 +30,7 @@ const (
 	// * The Grant.validateType function and test
 	// * The perms.topLevelTypes function
 	// * The scopes service collection actions for appropriate scopes
+	// * The Test_AnonRestrictions test
 )
 
 func (r Type) MarshalJSON() ([]byte, error) {

website/content/docs/concepts/security/permissions/assignable-permissions.mdx

@@ -24,33 +24,51 @@ know.
 
 #### Subactions
 
-Starting in Boundary 0.1.6, some subactions are supported. These actions have a
-format `top_level_action:subaction`, such as `read:self`. Being granted the top
-level action infers being granted all subactions. Thus, if a grant conveys
-`read`, it also matches the API actions `read` and `read:self`. However, if a
-grant conveys `read:self`, it will match the API action `read:self` but will not
-match `read`.
+Some subactions are supported. These actions have a format
+`top_level_action:subaction`, such as `read:self`. Being granted the top level
+action infers being granted all subactions. Thus, if a grant conveys `read`, it
+also matches the API actions `read` and `read:self`. However, if a grant conveys
+`read:self`, it will match the API action `read:self` but will not match `read`.
 
-#### The `no-op` Action
+#### The `no-op` Action and Listing Visibility
 
-Starting in Boundary 0.2.1 there is an action that can be granted called
-`no-op`. As might be apparent, `no-op` is not used for any real action in
-Boundary; the purpose of this action is for listing visibility. Boundary only
-shows resources in the output of a `list` action for which a user has at least
-one granted action. Thus, without `no-op`, in order for a resource to be visible
-in a `list` action, a different action such as `read` would have to be granted
-to a user. This could result in exposing more information than desired,
-especially in the case of listing scopes and authentication methods so that
-users can perform initial authentication to Boundary (that is, granting access
-to `u_anon`).
+There is an action that can be granted called `no-op`. As might be apparent,
+`no-op` is not used for any real action in Boundary; the purpose of this action
+is for listing visibility. Boundary only shows resources in the output of a
+`list` action for which a user has at least one granted action. Thus, without
+`no-op`, in order for a resource to be visible in a `list` action, a different
+action such as `read` would have to be granted to a user. This could result in
+exposing more information than desired, especially in the case of listing scopes
+and authentication methods so that users can perform initial authentication to
+Boundary (that is, granting access to `u_anon`).
 
 By granting the `no-op` action to users, they can see the resources in the
 output of a list command without needing other capability grants as well.
 
+### Anonymous User Restrictions
+
+Starting in Boundary 0.9.0, there are severe limits placed on the actions
+allowed to be assigned to the anonymous user `u_anon`. This is meant to avoid
+situations where an operator accidentally or mistakenly adds the anonymous user
+to a role that provides more privileges than might be intended for
+unauthenticated users.
+
+The set of actions is currently restricted to listing scopes and auth methods
+and authentication to auth methods (plus `no-op` actions for listing
+visibility). If further use-cases arise from user feedback this list can be
+expanded.
+
+Note: there is no special error message returned when access is denied due to it
+being disallowed to the anonymous user. It is still possible to assign grants to
+a role on which the anonymous user is a principal that can never match the
+anonymous user. In the future this may change, but trying to limit this
+introduces other restrictions on reasonable administrative workflows so an
+acceptable set of tradeoffs would need to be figured out.
+
 ### Output Fields
 
-Starting in Boundary 0.2.1, grant strings can contain an `output_fields` field.
-This allows fine-grained control over visibility of data returned to users.
+Grant strings can contain an `output_fields` field. This allows fine-grained
+control over visibility of data returned to users.
 
 In many cases, `output_fields` will not need to be set directly. However, an
 example in the form of some history helps provide some context as to when this
@@ -116,20 +134,4 @@ composition, is equivalent to using Boundary's default; however the moment that
 grants start specifying output fields, it is composed from an empty set and thus
 nothing is contained unless explicitly specified. (An actual empty set is not
 currently supported, as we don't perform validation on the values given.
-However, this means setting `output_fields=none` is functionally equivalent!)
-
-#### Output Fields and Monitoring
-
-As another example, it's possible to get creative and use `output_fields` to
-allow services to keep tabs on various aspects of the system for monitoring
-purposes without needing to provision Boundary tokens to those services. For
-instance, by allowing access to list users to `u_anon` but restrict the set of
-fields to `id` only, a monitoring system could watch and alert if more users
-than expected are showing up in the system, while the IDs themselves are not
-really meaningful to any other caller that accesses the same endpoint.
-
-This will be especially useful once we introduce the ability to have CIDR
-restrictions against roles and/or grants, which are planned but not currently
-scheduled, as you can only have these grants apply to specific internal
-services, along with restricting the data that is returned for those services
-that do match.
+However, this means setting `output_fields=none` is functionally equivalent!)