diff --git a/CHANGELOG.md b/CHANGELOG.md index e574110a4..47e23c809 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,15 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. -## Pending +## Next + +### New and Improved + +* server: When performing recursive listing, `list` action is not longer + required to be granted to the calling user. Instead, the given scope acts as + the root point (so only results under that scope will be shown), and `list` + grant is evaluated per-scope. + [PR](https://github.com/hashicorp/boundary/pull/1016) ### Bug Fixes diff --git a/internal/auth/auth.go b/internal/auth/auth.go index fe11159dc..89ce07cb7 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -63,10 +63,11 @@ type RequestInfo struct { } type VerifyResults struct { - UserId string - AuthTokenId string - Error error - Scope *scopes.ScopeInfo + UserId string + AuthTokenId string + Error error + Scope *scopes.ScopeInfo + Authenticated bool // RoundTripValue can be set to allow the function performing authentication // (often accompanied by lookup(s)) to return a result of that lookup to the @@ -175,7 +176,8 @@ func Verify(ctx context.Context, opt ...Option) (ret VerifyResults) { } ret.AuthTokenId = v.requestInfo.PublicId - if !authResults.Allowed { + ret.Authenticated = authResults.Authenticated + if !authResults.Authorized { if v.requestInfo.DisableAuthzFailures { ret.Error = nil // TODO: Decide whether to remove this @@ -407,7 +409,8 @@ func (v verifier) performAuthCheck() (aclResults perms.ACLResults, userId string // At this point we don't need to look up grants since it's automatically allowed if v.requestInfo.TokenFormat == AuthTokenTypeRecoveryKms { - aclResults.Allowed = true + aclResults.Authenticated = true + aclResults.Authorized = true retErr = nil return } @@ -442,6 +445,11 @@ func (v verifier) performAuthCheck() (aclResults perms.ACLResults, userId string retAcl = perms.NewACL(parsedGrants...) aclResults = retAcl.Allowed(*v.res, v.act) + // 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 + // grants successfully loaded. + aclResults.Authenticated = true retErr = nil return } @@ -469,9 +477,11 @@ func (r *VerifyResults) fetchActions(ctx context.Context, id string, typ resourc opts := getOpts(opt...) res := opts.withResource + // If not passed in, use what's already been populated through verification if res == nil { res = r.v.res } + // If this is being called directly we may not have a resource yet if res == nil { res = new(perms.Resource) } @@ -484,7 +494,7 @@ func (r *VerifyResults) fetchActions(ctx context.Context, id string, typ resourc ret := make(action.ActionSet, 0, len(availableActions)) for _, act := range availableActions { - if r.v.acl.Allowed(*res, act).Allowed { + if r.v.acl.Allowed(*res, act).Authorized { ret = append(ret, act) } } diff --git a/internal/iam/options.go b/internal/iam/options.go index eb8d50132..ae0238e6e 100644 --- a/internal/iam/options.go +++ b/internal/iam/options.go @@ -19,7 +19,6 @@ type options struct { withPublicId string withName string withDescription string - withGroupGrants bool withLimit int withAutoVivify bool withGrantScopeId string @@ -36,7 +35,6 @@ func getDefaultOptions() options { withPublicId: "", withName: "", withDescription: "", - withGroupGrants: false, withLimit: 0, withAutoVivify: false, withGrantScopeId: "", @@ -44,13 +42,6 @@ func getDefaultOptions() options { } } -// WithGroupGrants provides and option to include group grants -func WithGroupGrants(enable bool) Option { - return func(o *options) { - o.withGroupGrants = enable - } -} - // WithPublicId provides an optional public id func WithPublicId(id string) Option { return func(o *options) { diff --git a/internal/perms/acl.go b/internal/perms/acl.go index 59081e940..83c417153 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -30,7 +30,8 @@ type ACL struct { // pass more detailed information along in the future if we want. It was useful // in Vault, may be useful here. type ACLResults struct { - Allowed bool + Authenticated bool + Authorized bool // This is included but unexported for testing/debugging scopeMap map[string][]Grant @@ -102,7 +103,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { aType != action.List && aType != action.Create: - results.Allowed = true + results.Authorized = true return // type=;actions= when action is list(:self) or @@ -116,7 +117,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { (aType == action.List || aType == action.Create): - results.Allowed = true + results.Authorized = true return // id=*;type=;actions= where type cannot be @@ -126,7 +127,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { (grant.typ == r.Type || grant.typ == resource.All): - results.Allowed = true + results.Authorized = true return // id=;type=;actions= where type can be a @@ -137,7 +138,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { (grant.typ == r.Type || grant.typ == resource.All) && !topLevelType(r.Type): - results.Allowed = true + results.Authorized = true return } } diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index 8573f3ff1..2a1d96562 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -16,17 +16,17 @@ func Test_ACLAllowed(t *testing.T) { scope string grants []string } - type actionAllowed struct { - action action.Type - allowed bool + type actionAuthorized struct { + action action.Type + authorized bool } type input struct { - name string - scopeGrants []scopeGrant - resource Resource - actionsAllowed []actionAllowed - userId string - accountId string + name string + scopeGrants []scopeGrant + resource Resource + actionsAuthorized []actionAuthorized + userId string + accountId string } // A set of common grants to use in the following tests @@ -71,7 +71,7 @@ func Test_ACLAllowed(t *testing.T) { { name: "no grants", resource: Resource{ScopeId: "foo", Id: "bar", Type: resource.HostCatalog}, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Create}, {action: action.Read}, }, @@ -80,7 +80,7 @@ func Test_ACLAllowed(t *testing.T) { name: "no overlap", resource: Resource{ScopeId: "foo", Id: "bar", Type: resource.HostCatalog}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Create}, {action: action.Read}, }, @@ -89,8 +89,8 @@ func Test_ACLAllowed(t *testing.T) { name: "top level create with type only", resource: Resource{ScopeId: "o_a", Type: resource.HostCatalog}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Create, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Create, authorized: true}, {action: action.Delete}, }, }, @@ -98,7 +98,7 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and id no matching action", resource: Resource{ScopeId: "o_a", Id: "a_foo", Type: resource.Role}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Update}, {action: action.Delete}, }, @@ -107,9 +107,9 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and id and matching action", resource: Resource{ScopeId: "o_a", Id: "a_bar"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Read, allowed: true}, - {action: action.Update, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Read, authorized: true}, + {action: action.Update, authorized: true}, {action: action.Delete}, }, }, @@ -117,17 +117,17 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and type and all action with valid pin", resource: Resource{ScopeId: "o_b", Pin: "mypin", Type: resource.Host}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Read, allowed: true}, - {action: action.Update, allowed: true}, - {action: action.Delete, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Read, authorized: true}, + {action: action.Update, authorized: true}, + {action: action.Delete, authorized: true}, }, }, { name: "matching scope and type and all action but bad pin", resource: Resource{ScopeId: "o_b", Pin: "notmypin", Type: resource.Host}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Read}, {action: action.Update}, {action: action.Delete}, @@ -137,9 +137,9 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and id and some action", resource: Resource{ScopeId: "o_b", Id: "myhost", Type: resource.HostSet}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.List, allowed: true}, - {action: action.Create, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.List, authorized: true}, + {action: action.Create, authorized: true}, {action: action.AddHosts}, }, }, @@ -147,7 +147,7 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and id and all action but bad specifier", resource: Resource{ScopeId: "o_b", Id: "id_g"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Read}, {action: action.Update}, {action: action.Delete}, @@ -157,7 +157,7 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and not matching type", resource: Resource{ScopeId: "o_a", Type: resource.HostCatalog}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Update}, {action: action.Delete}, }, @@ -166,9 +166,9 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and matching type", resource: Resource{ScopeId: "o_a", Type: resource.HostSet}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.List, allowed: true}, - {action: action.Create, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.List, authorized: true}, + {action: action.Create, authorized: true}, {action: action.Delete}, }, }, @@ -176,7 +176,7 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope, type, action, random id and bad pin", resource: Resource{ScopeId: "o_a", Id: "anything", Type: resource.HostCatalog, Pin: "a_bar"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Update}, {action: action.Delete}, {action: action.Read}, @@ -186,7 +186,7 @@ func Test_ACLAllowed(t *testing.T) { name: "wrong scope and matching type", resource: Resource{ScopeId: "o_bad", Type: resource.HostSet}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.List}, {action: action.Create}, {action: action.Delete}, @@ -196,9 +196,9 @@ func Test_ACLAllowed(t *testing.T) { name: "any id", resource: Resource{ScopeId: "o_b", Type: resource.AuthMethod}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.List}, - {action: action.Authenticate, allowed: true}, + {action: action.Authenticate, authorized: true}, {action: action.Delete}, }, }, @@ -206,7 +206,7 @@ func Test_ACLAllowed(t *testing.T) { name: "bad templated user id", resource: Resource{ScopeId: "o_c"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.List}, {action: action.Authenticate}, {action: action.Delete}, @@ -217,9 +217,9 @@ func Test_ACLAllowed(t *testing.T) { name: "good templated user id", resource: Resource{ScopeId: "o_c", Id: "u_abcd1234"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Read, allowed: true}, - {action: action.Update, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Read, authorized: true}, + {action: action.Update, authorized: true}, }, userId: "u_abcd1234", }, @@ -227,7 +227,7 @@ func Test_ACLAllowed(t *testing.T) { name: "bad templated account id", resource: Resource{ScopeId: "o_c"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.List}, {action: action.Authenticate}, {action: action.Delete}, @@ -238,8 +238,8 @@ func Test_ACLAllowed(t *testing.T) { name: "good templated user id", resource: Resource{ScopeId: "o_c", Id: "apw_1234567890"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.ChangePassword, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.ChangePassword, authorized: true}, {action: action.Update}, }, accountId: "apw_1234567890", @@ -248,9 +248,9 @@ func Test_ACLAllowed(t *testing.T) { name: "all type", resource: Resource{ScopeId: "o_d", Type: resource.Account}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Create, allowed: true}, - {action: action.Update, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Create, authorized: true}, + {action: action.Update, authorized: true}, }, userId: "u_abcd1234", }, @@ -258,34 +258,34 @@ func Test_ACLAllowed(t *testing.T) { name: "list with top level list", resource: Resource{ScopeId: "o_a", Type: resource.Target}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.List, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.List, authorized: true}, }, }, { name: "list sessions with wildcard actions", resource: Resource{ScopeId: "o_d", Type: resource.Session}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.List, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.List, authorized: true}, }, }, { name: "read self with top level read", resource: Resource{ScopeId: "o_a", Id: "a_bar"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Read, allowed: true}, - {action: action.ReadSelf, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Read, authorized: true}, + {action: action.ReadSelf, authorized: true}, }, }, { name: "read self only", resource: Resource{ScopeId: "o_a", Id: "a_baz"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Read}, - {action: action.ReadSelf, allowed: true}, + {action: action.ReadSelf, authorized: true}, }, }, } @@ -301,8 +301,8 @@ 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, "action: %s, acl allowed: %t, test action allowed: %t", aa.action, acl.Allowed(test.resource, aa.action).Allowed, aa.allowed) + for _, aa := range test.actionsAuthorized { + assert.True(t, acl.Allowed(test.resource, aa.action).Authorized == aa.authorized, "action: %s, acl authorized: %t, test action authorized: %t", aa.action, acl.Allowed(test.resource, aa.action).Authorized, aa.authorized) } }) } diff --git a/internal/perms/grants.go b/internal/perms/grants.go index 7b98b734e..58f0e050b 100644 --- a/internal/perms/grants.go +++ b/internal/perms/grants.go @@ -362,7 +362,7 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { var allowed bool for k := range grant.actions { results := acl.Allowed(r, k) - if results.Allowed { + if results.Authorized { allowed = true } } diff --git a/internal/servers/controller/common/scopeids/scope_ids.go b/internal/servers/controller/common/scopeids/scope_ids.go index b0e7446df..e634fcb2b 100644 --- a/internal/servers/controller/common/scopeids/scope_ids.go +++ b/internal/servers/controller/common/scopeids/scope_ids.go @@ -4,14 +4,18 @@ import ( "context" "github.com/hashicorp/boundary/internal/auth" + "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/gen/controller/api/resources/scopes" + "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/perms" "github.com/hashicorp/boundary/internal/servers/controller/common" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" + "github.com/hashicorp/boundary/internal/types/scope" ) -// GetScopeIds, given common parameters for List calls, returns the set of scope +// GetListingScopeIds, given common parameters for List calls, returns the set of scope // IDs in which to search for resources. It also returns a memoized map of the // scopes to their info for populating returned values. // @@ -19,7 +23,7 @@ import ( // tests in the other service handlers test this function extensively as it // forms the basis for all recursive listing tests; see those tests for list // functionality in the various service handlers. -func GetScopeIds( +func GetListingScopeIds( // The context to use when listing in the DB, if required ctx context.Context, // An IAM repo function to use for a listing call, if required @@ -29,63 +33,146 @@ func GetScopeIds( // The scope ID to use, or to use as the starting point for a recursive // search rootScopeId string, + // The type of resource we are listing + typ resource.Type, // Whether or not the search should be recursive - recursive bool) ([]string, map[string]*scopes.ScopeInfo, error) { - // Get the list of scope IDs. If it's recursive, check ACLs. - var scopeIds []string + recursive bool, + // Whether to only return scopes with exact permissions, or whether parent + // scopes with appropriate permissions are sufficient + directOnly bool) ([]string, map[string]*scopes.ScopeInfo, error) { + const op = "GetListingScopeIds" + + // Validation + switch { + case typ == resource.Unknown: + return nil, nil, errors.New(errors.InvalidParameter, op, "unknown resource") + case repoFn == nil: + return nil, nil, errors.New(errors.InvalidParameter, op, "nil iam repo") + case rootScopeId == "": + return nil, nil, errors.New(errors.InvalidParameter, op, "missing root scope id") + case authResults.Scope == nil: + return nil, nil, errors.New(errors.InvalidParameter, op, "nil scope in auth results") + } + + // Base case: if not recursive, return the scope we were given and the + // already-looked-up info + if !recursive { + return []string{rootScopeId}, map[string]*scopes.ScopeInfo{rootScopeId: authResults.Scope}, nil + } + // This will be used to memoize scope info so we can put the right scope // info for each returned value - scopeInfoMap := map[string]*scopes.ScopeInfo{ - rootScopeId: authResults.Scope, - } - switch recursive { - case true: - repo, err := repoFn() - if err != nil { - return nil, nil, err - } - // Get all scopes recursively - scps, err := repo.ListScopesRecursively(ctx, rootScopeId) - if err != nil { - return nil, nil, err - } - scopeIds = make([]string, 0, len(scps)) - res := perms.Resource{ - Type: resource.Role, - } - // For each scope, see if we have permission to list on that scope - for _, scp := range scps { - scpId := scp.GetPublicId() - // We already checked the incoming ID so we can definitely add it - if scpId == authResults.Scope.Id { - scopeIds = append(scopeIds, scp.GetPublicId()) - continue - } - res.ScopeId = scpId - aSet := authResults.FetchActionSetForType(ctx, - resource.Role, - action.ActionSet{action.List}, - auth.WithResource(&res), - ) - // We only passed one action in, so anything other than that one - // action back should not be included. Assuming it's correct, add - // the scope ID for lookup and memoize the scope info if needed. - if len(aSet) == 1 && aSet[0] == action.List { - scopeIds = append(scopeIds, scpId) - if scopeInfoMap[scpId] == nil { - scopeInfo := &scopes.ScopeInfo{ - Id: scp.GetPublicId(), - Type: scp.GetType(), - Name: scp.GetName(), - Description: scp.GetDescription(), - } - scopeInfoMap[scpId] = scopeInfo - } - } - } + scopeInfoMap := map[string]*scopes.ScopeInfo{} - default: - scopeIds = []string{rootScopeId} + repo, err := repoFn() + if err != nil { + return nil, nil, err + } + // Get all scopes recursively. Start at global because we need to take into + // account permissions in parent scopes even if they want to scale back the + // returned information to a child scope and its children. + scps, err := repo.ListScopesRecursively(ctx, scope.Global.String()) + if err != nil { + return nil, nil, err + } + + res := perms.Resource{ + Type: typ, + } + // For each scope, see if we have permission to list that type in that + // scope + var deferredScopes []*iam.Scope + // Store whether global has list permission + var globalHasList bool + for _, scp := range scps { + scpId := scp.GetPublicId() + res.ScopeId = scpId + aSet := authResults.FetchActionSetForType(ctx, + // This is overridden by WithResource + resource.Unknown, + action.ActionSet{action.List}, + auth.WithResource(&res), + ) + switch len(aSet) { + case 0: + // Defer until we've read all scopes. We do this because if the + // ordering coming back isn't in parent-first ording our map + // lookup might fail. + if !directOnly { + deferredScopes = append(deferredScopes, scp) + } + case 1: + if aSet[0] != action.List { + return nil, nil, errors.New(errors.Internal, op, "unexpected action in set") + } + if scopeInfoMap[scpId] == nil { + scopeInfo := &scopes.ScopeInfo{ + Id: scp.GetPublicId(), + Type: scp.GetType(), + Name: scp.GetName(), + Description: scp.GetDescription(), + ParentScopeId: scp.GetParentId(), + } + scopeInfoMap[scpId] = scopeInfo + } + if scpId == scope.Global.String() { + globalHasList = true + } + default: + return nil, nil, errors.New(errors.Internal, op, "unexpected number of actions back in set") + } + } + + // Now go through these and see if a parent matches + for _, scp := range deferredScopes { + // If they had list on global scope anything else is automatically + // included; otherwise if they had list on the parent scope, this + // scope is included in the map and is sufficient here. + if globalHasList || scopeInfoMap[scp.GetParentId()] != nil { + scpId := scp.GetPublicId() + if scopeInfoMap[scpId] == nil { + scopeInfo := &scopes.ScopeInfo{ + Id: scp.GetPublicId(), + Type: scp.GetType(), + Name: scp.GetName(), + Description: scp.GetDescription(), + ParentScopeId: scp.GetParentId(), + } + scopeInfoMap[scpId] = scopeInfo + } + } + } + + // If we have nothing in scopeInfoMap at this point, we aren't authorized + // anywhere so return 403. + if len(scopeInfoMap) == 0 { + return nil, nil, handlers.ForbiddenError() + } + + // Now elide out any that aren't under the root scope ID + elideScopes := make([]string, 0, len(scopeInfoMap)) + for scpId, scp := range scopeInfoMap { + switch rootScopeId { + // If the root is global, it matches + case scope.Global.String(): + // If the current scope matches the root, it matches + case scpId: + // Or if the parent of this scope is the root (for orgs that would mean + // a root scope ID which is covered in the case above, so this is really + // projects matching an org used as the root) + case scp.GetParentScopeId(): + default: + elideScopes = append(elideScopes, scpId) + } + } + + for _, scpId := range elideScopes { + delete(scopeInfoMap, scpId) + } + + scopeIds := make([]string, 0, len(scopeInfoMap)) + for k := range scopeInfoMap { + scopeIds = append(scopeIds, k) } return scopeIds, scopeInfoMap, nil diff --git a/internal/servers/controller/common/scopeids/scope_ids_test.go b/internal/servers/controller/common/scopeids/scope_ids_test.go new file mode 100644 index 000000000..71a9b940b --- /dev/null +++ b/internal/servers/controller/common/scopeids/scope_ids_test.go @@ -0,0 +1,315 @@ +package scopeids_test + +import ( + "context" + "testing" + + "github.com/hashicorp/boundary/internal/auth" + "github.com/hashicorp/boundary/internal/authtoken" + "github.com/hashicorp/boundary/internal/db" + pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" + "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" + "github.com/hashicorp/boundary/internal/servers/controller/handlers/groups" + "github.com/hashicorp/boundary/internal/servers/controller/handlers/sessions" + "github.com/hashicorp/boundary/internal/session" + "github.com/hashicorp/boundary/internal/types/scope" + "github.com/stretchr/testify/require" +) + +// This test validates that we can perform recursive listing when there are no +// permissions on the parent scope against which the query is being run, but +// there are on child scopes. We use groups because groups are (one type of +// resource) valid in projects and we want to validate that the initial bugged +// behavior that used role permissions instead of the resource type under test +// is fixed. +func TestListingScopeIds(t *testing.T) { + conn, _ := db.TestSetup(t, "postgres") + rw := db.New(conn) + wrap := db.TestWrapper(t) + kms := kms.TestKms(t, conn, wrap) + iamRepo := iam.TestRepo(t, conn, wrap) + iamRepoFn := func() (*iam.Repository, error) { + return iamRepo, nil + } + authTokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*servers.Repository, error) { + return servers.NewRepository(rw, rw, kms) + } + s, err := groups.NewService(iamRepoFn) + require.NoError(t, err) + + sessionsRepoFn := func() (*session.Repository, error) { + return session.NewRepository(rw, rw, kms) + } + sess, err := sessions.NewService(sessionsRepoFn, iamRepoFn) + require.NoError(t, err) + + tcs := []struct { + name string + globalGrants []string + orgGrants []string + projGrants []string + globalGroups int + orgGroups int + projGroups int + users []string + wantErr error + expCount int + recurseFrom string + }{ + { + name: "no perms, start at global", + recurseFrom: "global", + wantErr: handlers.ForbiddenError(), + }, + { + name: "no perms, start at org", + recurseFrom: "org", + wantErr: handlers.ForbiddenError(), + }, + { + name: "no perms, start at project", + recurseFrom: "project", + wantErr: handlers.ForbiddenError(), + }, + { + name: "perms on global, no groups", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + recurseFrom: "global", + }, + { + name: "perms on org, no groups", + orgGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + recurseFrom: "org", + }, + { + name: "perms on project, no groups", + projGrants: []string{"id=*;type=group;actions=list,read"}, + recurseFrom: "project", + }, + { + name: "perms on global, none in org", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 6, + recurseFrom: "global", + }, + { + name: "perms on global, none in org, start at org", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "org", + }, + { + name: "perms on global, with org", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + orgGrants: []string{"id=*;type=group;actions=read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 9, + recurseFrom: "global", + }, + { + name: "perms on global, with org, start at org", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + orgGrants: []string{"id=*;type=group;actions=read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 8, + recurseFrom: "org", + }, + { + name: "perms on global, start at project", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + orgGrants: []string{"id=*;type=group;actions=read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "project", + }, + { + name: "perms on org, start at global, no read on org", + orgGrants: []string{"id=*;type=group;actions=list"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "global", + }, + { + name: "perms on org, start at global, read on org", + orgGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 8, + recurseFrom: "global", + }, + { + name: "perms on org, start at org", + orgGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 8, + recurseFrom: "org", + }, + { + name: "perms on org, start at project", + orgGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "project", + }, + { + name: "perms on proj", + projGrants: []string{"id=*;type=group;actions=list,read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "project", + }, + { + name: "perms on proj, start at org", + projGrants: []string{"id=*;type=group;actions=list,read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "org", + }, + { + name: "perms on proj, start at global", + projGrants: []string{"id=*;type=group;actions=list,read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "global", + }, + } + // Each test starts with a new set of scopes and new users/roles, which + // ensures we don't clash and doesn't require us to deal with cleanup. Plus + // it means subsequent tests are ensuring that only the scopes that the + // current user should see are seen. + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + o, p := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, o.GetPublicId()) + ctx := auth.NewVerifierContext(context.Background(), + nil, + iamRepoFn, + authTokenRepoFn, + serversRepoFn, + kms, + auth.RequestInfo{ + Token: at.GetToken(), + TokenFormat: auth.AuthTokenTypeBearer, + PublicId: at.GetPublicId(), + }) + + // Clean up scopes between tests + defer func() { + _, err := iamRepo.DeleteScope(ctx, o.GetPublicId()) + require.NoError(err) + }() + + for i := 0; i < tc.globalGroups; i++ { + g := iam.TestGroup(t, conn, scope.Global.String()) + defer func() { + _, err := iamRepo.DeleteGroup(ctx, g.GetPublicId()) + require.NoError(err) + }() + } + for i := 0; i < tc.orgGroups; i++ { + iam.TestGroup(t, conn, o.GetPublicId()) + } + for i := 0; i < tc.projGroups; i++ { + iam.TestGroup(t, conn, p.GetPublicId()) + } + + for i, grants := range [][]string{tc.globalGrants, tc.orgGrants, tc.projGrants} { + if len(grants) > 0 { + pubId := scope.Global.String() + switch i { + case 1: + pubId = o.GetPublicId() + case 2: + pubId = p.GetPublicId() + } + role := iam.TestRole(t, conn, pubId) + // Clean up global between tests + if pubId == scope.Global.String() { + defer func() { + _, err := iamRepo.DeleteRole(ctx, role.GetPublicId()) + require.NoError(err) + }() + } + for _, grant := range grants { + iam.TestRoleGrant(t, conn, role.GetPublicId(), grant) + } + iam.TestUserRole(t, conn, role.GetPublicId(), at.GetIamUserId()) + } + } + var startScope string + switch tc.recurseFrom { + case "global": + startScope = scope.Global.String() + case "org": + startScope = o.GetPublicId() + case "project": + startScope = p.GetPublicId() + default: + t.Fatal("unknown start scope") + } + out, err := s.ListGroups(ctx, &pbs.ListGroupsRequest{ + Recursive: true, + ScopeId: startScope, + }) + if tc.wantErr != nil { + require.Error(err) + require.Nil(out) + require.Equal(tc.wantErr.Error(), err.Error()) + } else { + require.NoError(err) + require.NotNil(out) + require.Len(out.Items, tc.expCount) + } + + sessOut, err := sess.ListSessions(ctx, &pbs.ListSessionsRequest{ + Recursive: true, + ScopeId: startScope, + }) + require.NoError(err) + require.NotNil(sessOut) + }) + } +} diff --git a/internal/servers/controller/handlers/authmethods/authmethod_service.go b/internal/servers/controller/handlers/authmethods/authmethod_service.go index 73ad77007..7e9296e3c 100644 --- a/internal/servers/controller/handlers/authmethods/authmethod_service.go +++ b/internal/servers/controller/handlers/authmethods/authmethod_service.go @@ -128,14 +128,26 @@ func (s Service) ListAuthMethods(ctx context.Context, req *pbs.ListAuthMethodsRe } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.iamRepoFn, authResults, req.GetScopeId(), resource.AuthMethod, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListAuthMethodsResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/authtokens/authtoken_service.go b/internal/servers/controller/handlers/authtokens/authtoken_service.go index 4fcd1f1d7..671b82b29 100644 --- a/internal/servers/controller/handlers/authtokens/authtoken_service.go +++ b/internal/servers/controller/handlers/authtokens/authtoken_service.go @@ -61,14 +61,26 @@ func (s Service) ListAuthTokens(ctx context.Context, req *pbs.ListAuthTokensRequ } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.iamRepoFn, authResults, req.GetScopeId(), resource.AuthToken, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListAuthTokensResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/errors.go b/internal/servers/controller/handlers/errors.go index 8ea725c1b..357981e94 100644 --- a/internal/servers/controller/handlers/errors.go +++ b/internal/servers/controller/handlers/errors.go @@ -90,24 +90,28 @@ func NotFoundErrorf(msg string, a ...interface{}) error { } } +var unauthorizedError = &apiError{ + status: http.StatusForbidden, + inner: &pb.Error{ + Kind: codes.PermissionDenied.String(), + Message: "Forbidden.", + }, +} + func ForbiddenError() error { - return &apiError{ - status: http.StatusForbidden, - inner: &pb.Error{ - Kind: codes.PermissionDenied.String(), - Message: "Forbidden.", - }, - } + return unauthorizedError +} + +var unauthenticatedError = &apiError{ + status: http.StatusUnauthorized, + inner: &pb.Error{ + Kind: codes.Unauthenticated.String(), + Message: "Unauthenticated, or invalid token.", + }, } func UnauthenticatedError() error { - return &apiError{ - status: http.StatusUnauthorized, - inner: &pb.Error{ - Kind: codes.Unauthenticated.String(), - Message: "Unauthenticated, or invalid token.", - }, - } + return unauthenticatedError } func InvalidArgumentErrorf(msg string, fields map[string]string) error { diff --git a/internal/servers/controller/handlers/groups/group_service.go b/internal/servers/controller/handlers/groups/group_service.go index 5e7c4264d..7298fead4 100644 --- a/internal/servers/controller/handlers/groups/group_service.go +++ b/internal/servers/controller/handlers/groups/group_service.go @@ -75,14 +75,26 @@ func (s Service) ListGroups(ctx context.Context, req *pbs.ListGroupsRequest) (*p } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.repoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.repoFn, authResults, req.GetScopeId(), resource.Group, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListGroupsResponse{}, nil + } gl, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go b/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go index 38ed3641b..0acd93c35 100644 --- a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go +++ b/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go @@ -111,14 +111,26 @@ func (s Service) ListHostCatalogs(ctx context.Context, req *pbs.ListHostCatalogs } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.iamRepoFn, authResults, req.GetScopeId(), resource.HostCatalog, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListHostCatalogsResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/roles/role_service.go b/internal/servers/controller/handlers/roles/role_service.go index df43e7a0e..a6f8f09f2 100644 --- a/internal/servers/controller/handlers/roles/role_service.go +++ b/internal/servers/controller/handlers/roles/role_service.go @@ -78,14 +78,26 @@ func (s Service) ListRoles(ctx context.Context, req *pbs.ListRolesRequest) (*pbs } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.repoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.repoFn, authResults, req.GetScopeId(), resource.Role, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListRolesResponse{}, nil + } items, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/scopes/scope_service.go b/internal/servers/controller/handlers/scopes/scope_service.go index 19e0f6f54..fbf37023a 100644 --- a/internal/servers/controller/handlers/scopes/scope_service.go +++ b/internal/servers/controller/handlers/scopes/scope_service.go @@ -133,14 +133,26 @@ func (s Service) ListScopes(ctx context.Context, req *pbs.ListScopesRequest) (*p } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.repoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.repoFn, authResults, req.GetScopeId(), resource.Scope, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListScopesResponse{}, nil + } pl, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/sessions/session_service.go b/internal/servers/controller/handlers/sessions/session_service.go index 4e09633f8..d89352a1a 100644 --- a/internal/servers/controller/handlers/sessions/session_service.go +++ b/internal/servers/controller/handlers/sessions/session_service.go @@ -100,12 +100,26 @@ func (s Service) ListSessions(ctx context.Context, req *pbs.ListSessionsRequest) authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds(ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds(ctx, + s.iamRepoFn, authResults, req.GetScopeId(), resource.Session, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListSessionsResponse{}, nil + } seslist, err := s.listFromRepo(ctx, session.WithScopeIds(scopeIds)) if err != nil { diff --git a/internal/servers/controller/handlers/targets/target_service.go b/internal/servers/controller/handlers/targets/target_service.go index 716f9f110..e7b6f6dde 100644 --- a/internal/servers/controller/handlers/targets/target_service.go +++ b/internal/servers/controller/handlers/targets/target_service.go @@ -122,14 +122,26 @@ func (s Service) ListTargets(ctx context.Context, req *pbs.ListTargetsRequest) ( } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.iamRepoFn, authResults, req.GetScopeId(), resource.Target, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListTargetsResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/users/user_service.go b/internal/servers/controller/handlers/users/user_service.go index 3f5a41a71..15a68f5ae 100644 --- a/internal/servers/controller/handlers/users/user_service.go +++ b/internal/servers/controller/handlers/users/user_service.go @@ -76,14 +76,26 @@ func (s Service) ListUsers(ctx context.Context, req *pbs.ListUsersRequest) (*pbs } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.repoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.repoFn, authResults, req.GetScopeId(), resource.User, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListUsersResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil {