Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
97 changes: 95 additions & 2 deletions pkg/authorization/authorizer/scope/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"k8s.io/kubernetes/pkg/util/sets"

authorizationapi "github.com/openshift/origin/pkg/authorization/api"
"github.com/openshift/origin/pkg/authorization/authorizer"
"github.com/openshift/origin/pkg/client"
oauthapi "github.com/openshift/origin/pkg/oauth/api"
projectapi "github.com/openshift/origin/pkg/project/api"
Expand Down Expand Up @@ -48,6 +49,41 @@ func ScopesToRules(scopes []string, namespace string, clusterPolicyGetter client
return rules, kutilerrors.NewAggregate(errors)
}

// ScopesToVisibleNamespaces returns a list of namespaces that the provided scopes have "get" access to.
// This exists only to support efficiently list/watch of projects (ACLed namespaces)
func ScopesToVisibleNamespaces(scopes []string, clusterPolicyGetter client.ClusterPolicyLister) (sets.String, error) {
if len(scopes) == 0 {
return sets.NewString("*"), nil
}

visibleNamespaces := sets.String{}

errors := []error{}
for _, scope := range scopes {
found := false

for _, evaluator := range ScopeEvaluators {
if evaluator.Handles(scope) {
found = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break ranging over evaluators or let them all have a shot at it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break ranging over evaluators or let them all have a shot at it?

sure.

allowedNamespaces, err := evaluator.ResolveGettableNamespaces(scope, clusterPolicyGetter)
if err != nil {
errors = append(errors, err)
continue
}

visibleNamespaces.Insert(allowedNamespaces...)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we now contain *, return early?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we now contain *, return early?

That assumes this knows how the results will be used. I'd rather not, but if you feel strongly I won't fight.

break
}
}

if !found {
errors = append(errors, fmt.Errorf("no scope evaluator found for %q", scope))
}
}

return visibleNamespaces, kutilerrors.NewAggregate(errors)
}

const (
UserIndicator = "user:"
ClusterRoleIndicator = "role:"
Expand All @@ -61,6 +97,7 @@ type ScopeEvaluator interface {
Describe(scope string) string
Validate(scope string) error
ResolveRules(scope, namespace string, clusterPolicyGetter client.ClusterPolicyLister) ([]authorizationapi.PolicyRule, error)
ResolveGettableNamespaces(scope string, clusterPolicyGetter client.ClusterPolicyLister) ([]string, error)
}

// ScopeEvaluators map prefixes to a function that handles that prefix
Expand Down Expand Up @@ -134,7 +171,7 @@ func (userEvaluator) ResolveRules(scope, namespace string, clusterPolicyGetter c
}, nil
case UserListProject:
return []authorizationapi.PolicyRule{
{Verbs: sets.NewString("list"), APIGroups: []string{projectapi.GroupName}, Resources: sets.NewString("projects")},
{Verbs: sets.NewString("list", "watch"), APIGroups: []string{projectapi.GroupName}, Resources: sets.NewString("projects")},
}, nil
case UserFull:
return []authorizationapi.PolicyRule{
Expand All @@ -146,6 +183,15 @@ func (userEvaluator) ResolveRules(scope, namespace string, clusterPolicyGetter c
}
}

func (userEvaluator) ResolveGettableNamespaces(scope string, clusterPolicyGetter client.ClusterPolicyLister) ([]string, error) {
switch scope {
case UserFull:
return []string{"*"}, nil
default:
return []string{}, nil
}
}

// escalatingScopeResources are resources that are considered escalating for scope evaluation
var escalatingScopeResources = []unversioned.GroupResource{
{Group: kapi.GroupName, Resource: "secrets"},
Expand Down Expand Up @@ -175,6 +221,12 @@ func (e clusterRoleEvaluator) parseScope(scope string) (string /*role name*/, st
if !e.Handles(scope) {
return "", "", false, fmt.Errorf("bad format for scope %v", scope)
}
return ParseClusterRoleScope(scope)
}
func ParseClusterRoleScope(scope string) (string /*role name*/, string /*namespace*/, bool /*escalating*/, error) {
if !strings.HasPrefix(scope, ClusterRoleIndicator) {
return "", "", false, fmt.Errorf("bad format for scope %v", scope)
}
escalating := false
if strings.HasSuffix(scope, ":!") {
escalating = true
Expand Down Expand Up @@ -214,7 +266,7 @@ func (e clusterRoleEvaluator) Describe(scope string) string {
}

func (e clusterRoleEvaluator) ResolveRules(scope, namespace string, clusterPolicyGetter client.ClusterPolicyLister) ([]authorizationapi.PolicyRule, error) {
roleName, scopeNamespace, escalating, err := e.parseScope(scope)
_, scopeNamespace, _, err := e.parseScope(scope)
if err != nil {
return nil, err
}
Expand All @@ -224,6 +276,16 @@ func (e clusterRoleEvaluator) ResolveRules(scope, namespace string, clusterPolic
return []authorizationapi.PolicyRule{}, nil
}

return e.resolveRules(scope, clusterPolicyGetter)
}

// resolveRules doesn't enforce namespace checks
func (e clusterRoleEvaluator) resolveRules(scope string, clusterPolicyGetter client.ClusterPolicyLister) ([]authorizationapi.PolicyRule, error) {
roleName, _, escalating, err := e.parseScope(scope)
if err != nil {
return nil, err
}

policy, err := clusterPolicyGetter.Get("default")
if err != nil {
return nil, err
Expand Down Expand Up @@ -252,6 +314,37 @@ func (e clusterRoleEvaluator) ResolveRules(scope, namespace string, clusterPolic
return rules, nil
}

func (e clusterRoleEvaluator) ResolveGettableNamespaces(scope string, clusterPolicyGetter client.ClusterPolicyLister) ([]string, error) {
_, scopeNamespace, _, err := e.parseScope(scope)
if err != nil {
return nil, err
}
rules, err := e.resolveRules(scope, clusterPolicyGetter)
if err != nil {
return nil, err
}

attributes := authorizer.DefaultAuthorizationAttributes{
APIGroup: kapi.GroupName,
Verb: "get",
Resource: "namespaces",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally, there's a ResourceName in the attributes we pass to RuleMatches()... trying to reason about what it means when there isn't one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally, there's a ResourceName in the attributes we pass to RuleMatches()... trying to reason about what it means when there isn't one

If a rule has no resource names, it can match. If the rule has a name, someone would have had to create a rule with name == "". I can update validation to prevent that in new roles if you like, but its a pretty insane thing to do.

Most rules won't have names on namespace rules. Those that do will be honored. Those that don't behave properly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

}

errors := []error{}
for _, rule := range rules {
matches, err := attributes.RuleMatches(rule)
Copy link
Copy Markdown
Contributor

@liggitt liggitt Aug 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we also need to check that one of the following is true?

  • rule.ResourceNames is empty
  • rule.ResourceNames contains scopeNamespace
  • rule.ResourceNames contains "*"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rule.ResourceNames contains scopeNamespace

It won't because the rule is only applied in a given namespace. Admin has "get" on "namespace" and the rule is only checked if the role is bound in that namespace.

rule.ResourceNames is empty

I think the "I said he could look at an invalid namespace name" is a small enough edge to accept. That role never worked right.

rule.ResourceNames contains "*"

* isn't special for resourceNames. Empty list means all, * tries to match the name *. That's my "do awesome safe things" username.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just checking

if err != nil {
errors = append(errors, err)
continue
}
if matches {
return []string{scopeNamespace}, nil
}
}

return []string{}, kutilerrors.NewAggregate(errors)
}

// TODO: direct deep copy needing a cloner is something that should be fixed upstream
var localCloner = conversion.NewCloner()

Expand Down
18 changes: 16 additions & 2 deletions pkg/project/auth/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import (
utilwait "k8s.io/kubernetes/pkg/util/wait"
"k8s.io/kubernetes/pkg/watch"

authorizationapi "github.com/openshift/origin/pkg/authorization/api"
"github.com/openshift/origin/pkg/authorization/authorizer/scope"
"github.com/openshift/origin/pkg/client"
)

Expand Down Expand Up @@ -228,6 +230,10 @@ func (ac *AuthorizationCache) RemoveWatcher(watcher CacheWatcher) {
}
}

func (ac *AuthorizationCache) GetClusterPolicyLister() client.SyncedClusterPoliciesListerInterface {
return ac.clusterPolicyLister
}

// synchronizeNamespaces synchronizes access over each namespace and returns a set of namespace names that were looked at in last sync
func (ac *AuthorizationCache) synchronizeNamespaces(userSubjectRecordStore cache.Store, groupSubjectRecordStore cache.Store, reviewRecordStore cache.Store) sets.String {
namespaceSet := sets.NewString()
Expand Down Expand Up @@ -430,14 +436,22 @@ func (ac *AuthorizationCache) List(userInfo user.Info) (*kapi.NamespaceList, err
}
}

allowedNamespaces, err := scope.ScopesToVisibleNamespaces(userInfo.GetExtra()[authorizationapi.ScopesKey], ac.clusterPolicyLister.ClusterPolicies())
if err != nil {
return nil, err
}

namespaceList := &kapi.NamespaceList{}
for key := range keys {
namespace, exists, err := ac.namespaceStore.GetByKey(key)
namespaceObj, exists, err := ac.namespaceStore.GetByKey(key)
if err != nil {
return nil, err
}
if exists {
namespaceList.Items = append(namespaceList.Items, *namespace.(*kapi.Namespace))
namespace := *namespaceObj.(*kapi.Namespace)
if allowedNamespaces.Has("*") || allowedNamespaces.Has(namespace.Name) {
namespaceList.Items = append(namespaceList.Items, namespace)
}
}
}
return namespaceList, nil
Expand Down
21 changes: 13 additions & 8 deletions pkg/project/auth/watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ type WatchableCache interface {

// userProjectWatcher converts a native etcd watch to a watch.Interface.
type userProjectWatcher struct {
username string
groups []string
user user.Info
// visibleNamespaces are the namespaces that the scopes allow
visibleNamespaces sets.String

// cacheIncoming is a buffered channel used for notification to watcher. If the buffer fills up,
// then the watcher will be removed and the connection will be broken.
Expand Down Expand Up @@ -69,9 +70,8 @@ var (
watchChannelHWM etcd.HighWaterMark
)

func NewUserProjectWatcher(username string, groups []string, projectCache *projectcache.ProjectCache, authCache WatchableCache, includeAllExistingProjects bool) *userProjectWatcher {
userInfo := &user.DefaultInfo{Name: username, Groups: groups}
namespaces, _ := authCache.List(userInfo)
func NewUserProjectWatcher(user user.Info, visibleNamespaces sets.String, projectCache *projectcache.ProjectCache, authCache WatchableCache, includeAllExistingProjects bool) *userProjectWatcher {
namespaces, _ := authCache.List(user)
knownProjects := map[string]string{}
for _, namespace := range namespaces.Items {
knownProjects[namespace.Name] = namespace.ResourceVersion
Expand All @@ -84,8 +84,8 @@ func NewUserProjectWatcher(username string, groups []string, projectCache *proje
}

w := &userProjectWatcher{
username: username,
groups: groups,
user: user,
visibleNamespaces: visibleNamespaces,

cacheIncoming: make(chan watch.Event, 1000),
cacheError: make(chan error, 1),
Expand All @@ -107,7 +107,12 @@ func NewUserProjectWatcher(username string, groups []string, projectCache *proje
}

func (w *userProjectWatcher) GroupMembershipChanged(namespaceName string, users, groups sets.String) {
hasAccess := users.Has(w.username) || groups.HasAny(w.groups...)
if !w.visibleNamespaces.Has("*") && !w.visibleNamespaces.Has(namespaceName) {
// this user is scoped to a level that shouldn't see this update
return
}

hasAccess := users.Has(w.user.GetName()) || groups.HasAny(w.user.GetGroups()...)
_, known := w.knownProjects[namespaceName]

switch {
Expand Down
4 changes: 2 additions & 2 deletions pkg/project/auth/watch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
projectcache "github.com/openshift/origin/pkg/project/cache"
)

func newTestWatcher(user string, groups []string, namespaces ...*kapi.Namespace) (*userProjectWatcher, *fakeAuthCache) {
func newTestWatcher(username string, groups []string, namespaces ...*kapi.Namespace) (*userProjectWatcher, *fakeAuthCache) {
objects := []runtime.Object{}
for i := range namespaces {
objects = append(objects, namespaces[i])
Expand All @@ -27,7 +27,7 @@ func newTestWatcher(user string, groups []string, namespaces ...*kapi.Namespace)
projectCache.Run()
fakeAuthCache := &fakeAuthCache{}

return NewUserProjectWatcher(user, groups, projectCache, fakeAuthCache, false), fakeAuthCache
return NewUserProjectWatcher(&user.DefaultInfo{Name: username, Groups: groups}, sets.NewString("*"), projectCache, fakeAuthCache, false), fakeAuthCache
}

type fakeAuthCache struct {
Expand Down
9 changes: 8 additions & 1 deletion pkg/project/registry/project/proxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import (
"k8s.io/kubernetes/pkg/watch"

oapi "github.com/openshift/origin/pkg/api"
authorizationapi "github.com/openshift/origin/pkg/authorization/api"
"github.com/openshift/origin/pkg/authorization/authorizer/scope"
"github.com/openshift/origin/pkg/project/api"
projectapi "github.com/openshift/origin/pkg/project/api"
projectauth "github.com/openshift/origin/pkg/project/auth"
Expand Down Expand Up @@ -91,7 +93,12 @@ func (s *REST) Watch(ctx kapi.Context, options *kapi.ListOptions) (watch.Interfa

includeAllExistingProjects := (options != nil) && options.ResourceVersion == "0"

watcher := projectauth.NewUserProjectWatcher(userInfo.GetName(), userInfo.GetGroups(), s.projectCache, s.authCache, includeAllExistingProjects)
allowedNamespaces, err := scope.ScopesToVisibleNamespaces(userInfo.GetExtra()[authorizationapi.ScopesKey], s.authCache.GetClusterPolicyLister().ClusterPolicies())
if err != nil {
return nil, err
}

watcher := projectauth.NewUserProjectWatcher(userInfo, allowedNamespaces, s.projectCache, s.authCache, includeAllExistingProjects)
s.authCache.AddWatcher(watcher)

go watcher.Watch()
Expand Down
4 changes: 3 additions & 1 deletion test/cmd/authentication.sh
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ os::cmd::expect_success_and_text "oc get user/~ --token='${whoamitoken}'" "${use
os::cmd::expect_failure_and_text "oc get pods --token='${whoamitoken}' -n '${project}'" "prevent this action; User \"scoped-user\" cannot list pods in project \"${project}\""

listprojecttoken="$(oc process -f "${OS_ROOT}/test/testdata/authentication/scoped-token-template.yaml" TOKEN_PREFIX=listproject SCOPE=user:list-projects USER_NAME="${username}" USER_UID="${useruid}" | oc create -f - -o name | awk -F/ '{print $2}')"
os::cmd::expect_success_and_text "oc get projects --token='${listprojecttoken}'" "${project}"
# this token doesn't have rights to see any projects even though it can hit the list endpoint, so an empty list is correct
# we'll add another scope that allows listing all known projects even if this token has no other powers in them.
os::cmd::expect_success_and_not_text "oc get projects --token='${listprojecttoken}'" "${project}"
os::cmd::expect_failure_and_text "oc get user/~ --token='${listprojecttoken}'" 'prevent this action; User "scoped-user" cannot get users at the cluster scope'
os::cmd::expect_failure_and_text "oc get pods --token='${listprojecttoken}' -n '${project}'" "prevent this action; User \"scoped-user\" cannot list pods in project \"${project}\""

Expand Down
Loading