From d6362f9e5e7c96a797a5ccfcd1fa2a5f8660b53e Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Thu, 9 Jun 2022 11:30:01 -0400 Subject: [PATCH] Restrict anonymous user capabilities (#2156) --- CHANGELOG.md | 33 +++++-- internal/daemon/controller/auth/auth.go | 8 +- internal/perms/acl.go | 51 +++++++++-- internal/perms/acl_test.go | 91 ++++++++++++++++++- internal/perms/grants.go | 2 +- internal/perms/option.go | 20 +++- internal/perms/option_test.go | 43 +++++++++ internal/perms/output_fields_test.go | 2 +- internal/types/action/action.go | 4 + internal/types/resource/resource.go | 1 + .../permissions/assignable-permissions.mdx | 74 +++++++-------- 11 files changed, 266 insertions(+), 63 deletions(-) create mode 100644 internal/perms/option_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 891278c29..e18ae6897 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/daemon/controller/auth/auth.go b/internal/daemon/controller/auth/auth.go index 91c122bd1..add85456f 100644 --- a/internal/daemon/controller/auth/auth.go +++ b/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) } diff --git a/internal/perms/acl.go b/internal/perms/acl.go index c4e813c1d..50bb00bb2 100644 --- a/internal/perms/acl.go +++ b/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=;actions= where ID cannot be a wildcard; or // id=;output_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=;actions= 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= and no action. + // Case 3: type=;actions= 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= 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=;actions= where type cannot be // unknown but can be a wildcard to allow any resource at all; or // id=*;type=;output_fields= with no action. @@ -152,6 +190,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { found = true + // Case 5: // id=;type=;actions= where type can be a // wildcard and this this is operating on a non-top-level type. Same for // output fields only. diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index c14edd65a..f0af444f5 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" ) @@ -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) + } + } + } + }) + } +} diff --git a/internal/perms/grants.go b/internal/perms/grants.go index 405f7de2b..440ed0afb 100644 --- a/internal/perms/grants.go +++ b/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 } diff --git a/internal/perms/option.go b/internal/perms/option.go index ab779ddee..4aa29574b 100644 --- a/internal/perms/option.go +++ b/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 + } +} diff --git a/internal/perms/option_test.go b/internal/perms/option_test.go new file mode 100644 index 000000000..51c02071c --- /dev/null +++ b/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) + }) +} diff --git a/internal/perms/output_fields_test.go b/internal/perms/output_fields_test.go index 7a7080559..3ef0532f2 100644 --- a/internal/perms/output_fields_test.go +++ b/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) }) diff --git a/internal/types/action/action.go b/internal/types/action/action.go index e7d00aa7d..d7986c3a4 100644 --- a/internal/types/action/action.go +++ b/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{ diff --git a/internal/types/resource/resource.go b/internal/types/resource/resource.go index 45edab66f..b53785030 100644 --- a/internal/types/resource/resource.go +++ b/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) { diff --git a/website/content/docs/concepts/security/permissions/assignable-permissions.mdx b/website/content/docs/concepts/security/permissions/assignable-permissions.mdx index 9e9886a2e..b1f65df4d 100644 --- a/website/content/docs/concepts/security/permissions/assignable-permissions.mdx +++ b/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. \ No newline at end of file +However, this means setting `output_fields=none` is functionally equivalent!) \ No newline at end of file