Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the UI to correctly determine if a user has access to a resource #9473

Merged
merged 3 commits into from
Dec 21, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Introduce the CheckAccessToAnyResource method
  • Loading branch information
codingllama committed Dec 21, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 2e3853c05b4cf578afb537b8fcc89870a797d063
74 changes: 63 additions & 11 deletions lib/services/role.go
Original file line number Diff line number Diff line change
@@ -1799,6 +1799,35 @@ func (set RoleSet) String() string {
return fmt.Sprintf("roles %v", strings.Join(roleNames, ","))
}

// CheckAccessToAnyResource is used to determine if access is possible for an
// entire category of resources.
// Rules with a Where clause are handled differently by the method:
// - "allow" rules have their "where" clause always match, as it's assumed that
// there could be a resource that matches it.
// - "deny" rules have their "where" clause always fail, as it's assumed that
// there could be a resource that passes it.
// Do not use this method to check for access for specific resources, instead
// use CheckAccessToRule.
func (set RoleSet) CheckAccessToAnyResource(ctx RuleContext, namespace string, resource string, verb string, silent bool) error {
return set.checkAccessToRuleImpl(checkAccessParams{
ctx: ctx,
namespace: namespace,
resource: resource,
verb: verb,
allowWhere: boolParser(true), // always matches
denyWhere: boolParser(false), // never matches
silent: silent,
})
}

type boolParser bool

func (p boolParser) Parse(string) (interface{}, error) {
return predicate.BoolPredicate(func() bool {
return bool(p)
}), nil
}

// CheckAccessToRule checks if the RoleSet provides access in the given
// namespace to the specified resource and verb.
// silent controls whether the access violations are logged.
@@ -1807,35 +1836,58 @@ func (set RoleSet) CheckAccessToRule(ctx RuleContext, namespace string, resource
if err != nil {
return trace.Wrap(err)
}
actionsParser, err := NewActionsParser(ctx)

return set.checkAccessToRuleImpl(checkAccessParams{
ctx: ctx,
namespace: namespace,
resource: resource,
verb: verb,
allowWhere: whereParser,
denyWhere: whereParser,
silent: silent,
})
}

type checkAccessParams struct {
ctx RuleContext
namespace string
resource string
verb string
allowWhere, denyWhere predicate.Parser
silent bool
}

func (set RoleSet) checkAccessToRuleImpl(p checkAccessParams) error {
actionsParser, err := NewActionsParser(p.ctx)
if err != nil {
return trace.Wrap(err)
}

// check deny: a single match on a deny rule prohibits access
for _, role := range set {
matchNamespace, _ := MatchNamespace(role.GetNamespaces(types.Deny), types.ProcessNamespace(namespace))
matchNamespace, _ := MatchNamespace(role.GetNamespaces(types.Deny), types.ProcessNamespace(p.namespace))
if matchNamespace {
matched, err := MakeRuleSet(role.GetRules(types.Deny)).Match(whereParser, actionsParser, resource, verb)
matched, err := MakeRuleSet(role.GetRules(types.Deny)).Match(p.denyWhere, actionsParser, p.resource, p.verb)
if err != nil {
return trace.Wrap(err)
}
if matched {
if !silent {
if !p.silent {
log.WithFields(log.Fields{
trace.Component: teleport.ComponentRBAC,
}).Infof("Access to %v %v in namespace %v denied to %v: deny rule matched.",
verb, resource, namespace, role.GetName())
p.verb, p.resource, p.namespace, role.GetName())
}
return trace.AccessDenied("access denied to perform action %q on %q", verb, resource)
return trace.AccessDenied("access denied to perform action %q on %q", p.verb, p.resource)
}
}
}

// check allow: if rule matches, grant access to resource
for _, role := range set {
matchNamespace, _ := MatchNamespace(role.GetNamespaces(types.Allow), types.ProcessNamespace(namespace))
matchNamespace, _ := MatchNamespace(role.GetNamespaces(types.Allow), types.ProcessNamespace(p.namespace))
if matchNamespace {
match, err := MakeRuleSet(role.GetRules(types.Allow)).Match(whereParser, actionsParser, resource, verb)
match, err := MakeRuleSet(role.GetRules(types.Allow)).Match(p.allowWhere, actionsParser, p.resource, p.verb)
if err != nil {
return trace.Wrap(err)
}
@@ -1845,13 +1897,13 @@ func (set RoleSet) CheckAccessToRule(ctx RuleContext, namespace string, resource
}
}

if !silent {
if !p.silent {
log.WithFields(log.Fields{
trace.Component: teleport.ComponentRBAC,
}).Infof("Access to %v %v in namespace %v denied to %v: no allow rule matched.",
verb, resource, namespace, set)
p.verb, p.resource, p.namespace, set)
}
return trace.AccessDenied("access denied to perform action %q on %q", verb, resource)
return trace.AccessDenied("access denied to perform action %q on %q", p.verb, p.resource)
}

// ExtractConditionForIdentifier returns a restrictive filter expression
198 changes: 198 additions & 0 deletions lib/services/role_test.go
Original file line number Diff line number Diff line change
@@ -1491,6 +1491,204 @@ func TestCheckRuleAccess(t *testing.T) {
}
}

func TestCheckAccessToAnyResource(t *testing.T) {
// Examples from https://goteleport.com/docs/access-controls/reference/#rbac-for-sessions.
ownSessions, err := types.NewRole("own-sessions", types.RoleSpecV4{
Allow: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindSession},
Verbs: []string{types.VerbList, types.VerbRead},
Where: "contains(session.participants, user.metadata.name)",
},
},
},
})
require.NoError(t, err)
ownSSHSessions, err := types.NewRole("own-ssh-sessions", types.RoleSpecV4{
Allow: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindSSHSession},
Verbs: []string{types.Wildcard},
},
},
},
Deny: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindSSHSession},
Verbs: []string{types.VerbList, types.VerbRead, types.VerbUpdate, types.VerbDelete},
Where: "!contains(ssh_session.participants, user.metadata.name)",
},
},
},
})
require.NoError(t, err)

// Simple, all-or-nothing roles.
readAllSessions, err := types.NewRole("all-sessions", types.RoleSpecV4{
Allow: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindSession},
Verbs: []string{types.VerbList, types.VerbRead},
},
},
},
})
require.NoError(t, err)
allowSSHSessions, err := types.NewRole("all-ssh-sessions", types.RoleSpecV4{
Allow: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindSSHSession},
Verbs: []string{types.Wildcard},
},
},
},
})
require.NoError(t, err)
denySSHSessions, err := types.NewRole("deny-ssh-sessions", types.RoleSpecV4{
Deny: types.RoleConditions{
Rules: []types.Rule{
{
Resources: []string{types.KindSSHSession},
Verbs: []string{types.Wildcard},
},
},
},
})
require.NoError(t, err)

type checkAccessParams struct {
ctx Context
namespace string
resource string
verbs []string
}

tests := []struct {
name string
roles RoleSet
params *checkAccessParams
// wantRuleAccess fully evaluates where conditions, used to determine access
// to specific resources.
wantRuleAccess bool
// wantResourceAccess doesn't evaluate where conditions, used to determine
// access to a category of resources.
wantResourceAccess bool
}{
{
name: "global session list/read allowed",
roles: RoleSet{readAllSessions},
params: &checkAccessParams{
namespace: apidefaults.Namespace,
resource: types.KindSession,
verbs: []string{types.VerbList, types.VerbRead},
},
wantRuleAccess: true,
wantResourceAccess: true,
},
{
name: "own session list/read allowed",
roles: RoleSet{ownSessions}, // allowed despite "where" clause in allow rules
params: &checkAccessParams{
namespace: apidefaults.Namespace,
resource: types.KindSession,
verbs: []string{types.VerbList, types.VerbRead},
},
wantRuleAccess: false, // where condition needs specific resource
wantResourceAccess: true,
},
{
name: "session list/read denied",
roles: RoleSet{allowSSHSessions, denySSHSessions}, // none mention "session"
params: &checkAccessParams{
namespace: apidefaults.Namespace,
resource: types.KindSession,
verbs: []string{types.VerbList, types.VerbRead},
},
},
{
name: "session write denied",
roles: RoleSet{
readAllSessions, // readonly
allowSSHSessions, denySSHSessions, // none mention "session"
},
params: &checkAccessParams{
namespace: apidefaults.Namespace,
resource: types.KindSession,
verbs: []string{types.VerbUpdate, types.VerbDelete},
},
},
{
name: "global SSH session list/read allowed",
roles: RoleSet{allowSSHSessions},
params: &checkAccessParams{
namespace: apidefaults.Namespace,
resource: types.KindSSHSession,
verbs: []string{types.VerbList, types.VerbRead},
},
wantRuleAccess: true,
wantResourceAccess: true,
},
{
name: "own SSH session list/read allowed",
roles: RoleSet{ownSSHSessions}, // allowed despite "where" clause in deny rules
params: &checkAccessParams{
namespace: apidefaults.Namespace,
resource: types.KindSSHSession,
verbs: []string{types.VerbList, types.VerbRead},
},
wantRuleAccess: false, // where condition needs specific resource
wantResourceAccess: true,
},
{
name: "SSH session list/read denied",
roles: RoleSet{
allowSSHSessions, ownSSHSessions,
denySSHSessions, // unconditional deny, takes precedence
},
params: &checkAccessParams{
namespace: apidefaults.Namespace,
resource: types.KindSSHSession,
verbs: []string{types.VerbCreate, types.VerbList, types.VerbRead, types.VerbUpdate, types.VerbDelete},
},
},
{
name: "SSH session list/read denied - different role ordering",
roles: RoleSet{
allowSSHSessions,
denySSHSessions, // unconditional deny, takes precedence
ownSSHSessions,
},
params: &checkAccessParams{
namespace: apidefaults.Namespace,
resource: types.KindSSHSession,
verbs: []string{types.VerbCreate, types.VerbList, types.VerbRead, types.VerbUpdate, types.VerbDelete},
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
params := test.params
const silent = true
for _, verb := range params.verbs {
err := test.roles.CheckAccessToRule(&params.ctx, params.namespace, params.resource, verb, silent)
if gotAccess, wantAccess := err == nil, test.wantRuleAccess; gotAccess != wantAccess {
t.Errorf("CheckAccessToRule(verb=%q) returned err = %v=q, wantAccess = %v", verb, err, wantAccess)
}

err = test.roles.CheckAccessToAnyResource(&params.ctx, params.namespace, params.resource, verb, silent)
if gotAccess, wantAccess := err == nil, test.wantResourceAccess; gotAccess != wantAccess {
t.Errorf("CheckAccessToAnyResource(verb=%q) returned err = %q, wantAccess = %v", verb, err, wantAccess)
}
}
})
}
}

func TestCheckRuleSorting(t *testing.T) {
testCases := []struct {
name string