Introduce RestrictUsersAdmission plugin for collaboration #11743

Miciah · 2016-11-02T22:58:49Z

Add the RestrictUsersAdmission admission plugin for collaboration.

openshift-bot, please [test]!

Miciah · 2016-11-03T04:35:50Z

Looks like flake #10988. Please [test] again!

Miciah · 2016-11-03T15:35:18Z

Looks like flake #11649. Please [test] again!

Miciah · 2016-11-07T17:52:18Z

This PR implements proposal #9932 to restrict role bindings based on project annotations. The implementation is based on deads's WIP PR #9885. @deads2k, @smarterclayton, do you have time to review the approach in this PR?

deads2k · 2016-11-07T19:59:51Z

pkg/authorization/admission/restrictusers/restrictusers.go

+const (
+	// CollaboratorAnnotation is the key for an annotation on Namespace that
+	// specifies collaborators on the namespace.
+	CollaboratorAnnotation = "authorization.openshift.io/collaborators"


In 3.3, the kube infrastructure to register different groups was hopelessly broken. In 3.4, it's only mostly broken. In 3.5, it mostly works.

I think I'd prefer to see this as a separate API group and resource. That would allow "clean" management via oc and all of our usual tooling.

deads2k · 2016-11-07T20:02:48Z

pkg/authorization/admission/restrictusers/restrictusers.go

+
+	// Type is the type of collaborators against which this CollaboratorMatcher
+	// may be matched.
+	Type CollaboratorType `json:"type"`


Wrap these and call them Spec. I can envision a world where this controller driven and a controller drives updates to status to describe matches. That would prevent multiple lookup/matching calls on every API server and allow an admin to see who actually matches for something like content assist from a Status field.

// only one may be non-nil type CollaboratorMatcherSpec struct{ UserMatcher *UserMatcher GroupMatcher *GroupMatcher ServiceAccountMatcher *ServiceAccountMatcher } type UserMatcher struct{ Users []string UsersInTheseGroups []string UsersMatchingTheseSelectors []*unversioned.LabelSelector } type GroupMatcher struct{ Groups []string GroupsMatchingTheseSelectors []*unversioned.LabelSelector } type ServiceAccountMatcher struct{ ServiceAccounts []string ServiceAccountsInTheseNamespaces []string }

smarterclayton · 2016-11-08T08:04:18Z

The previous proposal never addressed the question of how an end user would get access to add other people to collaborate in a system where this was primarily to prevent abuse. How would this be used to allow for an online-like cluster (where collaboration was reasonable for arbitrary user emails)? Does this presuppose a higher level system for that? I'd like to be able to answer that question before we add that code.

deads2k · 2016-11-08T12:39:07Z

Does this presuppose a higher level system for that?

Yes. This assumes an external workflow could be built to handle workflow problems like "apply, approve" and "invite, accept" flows.

Miciah · 2016-11-18T19:02:12Z

pkg/security/api/v1/generated.proto

@@ -5,7 +5,7 @@ syntax = 'proto2';

 package ytb.ewangdian.workers.dev.openshift.origin.pkg.security.api.v1;

-import "github.com/openshift/origin/pkg/quota/api/v1/generated.proto";


I don't know why this is getting deleted.

Miciah · 2016-11-18T19:02:34Z

@deads2k, could you give this PR another look?

deads2k · 2016-11-18T19:20:01Z

pkg/authorization/admission/restrictusers/api/types.go

+// a string name on the name of a group to which the user belongs, or
+// a label selector.
+type UserMatcher struct {
+	Users                       []string


godoc all API fields.
I'm assuming these are literal string matches.

deads2k · 2016-11-18T19:21:08Z

pkg/authorization/admission/restrictusers/api/types.go

+type UserMatcher struct {
+	Users                       []string
+	UsersInTheseGroups          []string
+	UsersMatchingTheseSelectors []*unversioned.LabelSelector


I think Selector will do.

deads2k · 2016-11-18T19:21:24Z

pkg/authorization/admission/restrictusers/api/types.go

+// a label selector.
+type UserMatcher struct {
+	Users                       []string
+	UsersInTheseGroups          []string


Groups with good docs.

deads2k · 2016-11-18T19:22:23Z

pkg/authorization/admission/restrictusers/api/types.go

+// the service-account name or the name of the service account's namespace.
+type ServiceAccountMatcher struct {
+	ServiceAccounts                  []string
+	ServiceAccountsInTheseNamespaces []string


Namespaces?

deads2k · 2016-11-18T19:22:45Z

pkg/authorization/admission/restrictusers/api/types.go

+// ServiceAccountMatcher matches a service account by a string match on either
+// the service-account name or the name of the service account's namespace.
+type ServiceAccountMatcher struct {
+	ServiceAccounts                  []string


This needs to a slice of structs which have namespace and name.

deads2k · 2016-11-18T19:23:36Z

pkg/authorization/admission/restrictusers/api/types.go

@@ -0,0 +1,59 @@
+package api


The sub-package here is in admission. I was actually expecting you to add these directly to authorization/api/types.go and then read them inside of admission.

deads2k · 2016-11-18T19:30:17Z

pkg/authorization/admission/restrictusers/restrictusers.go

+	// Only bother checking deltas.
+	newSubjects := []kapi.ObjectReference{}
+	policy := authorizationapi.GetPolicyBindingName(binding.RoleRef.Namespace)
+	policybinding, _ := q.policyBindingLister.PolicyBindings(ns).Get(policy)


The old object is provided via a.GetOldObject(). Use that instead of a cached. copy.

deads2k · 2016-11-18T19:32:47Z

pkg/authorization/admission/restrictusers/restrictusers.go

+	for _, matcher := range matchersList.Items {
+		// TODO: Use a cache for users, groups, and serviceaccounts.
+		switch {
+		case matcher.Spec.UserMatcher != nil:


I'd like to see an interface defined that checks a subject. Something like Allowed(subject) error. I would be able to take a collaboratormatcherspec and get an allowed back out and then something that provides a union of subjectcheckers. It'll make it easier to read.

Are you suggesting something like the following?

func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) { // ... allowed := NewEmptyAllowed() allowed.SetNamespace(ns) allowed.SetClients(q.kclient, q.oclient) for _, matcher := range matchersList.Items { // NewAllowed(nil) is the same as NewEmptyAllowed(). // allowed.Merge(NewEmptyAllowed()) is a no-op. allowed = allowed.Merge(NewAllowed(matcher.UserMatcher)) allowed = allowed.Merge(NewAllowed(matcher.GroupMatcher)) allowed = allowed.Merge(NewAllowed(matcher.ServiceAccountMatcher)) } // ... }

For comparison, here is what I have now:

func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) { // ... checker, err := NewRoleBindingRestrictions(ns, q.kclient, q.oclient) if err != nil { return admission.NewForbidden(a, err) } for _, matcher := range matchersList.Items { checker.AddMatcher(&matcher) } // ... }

The latter seems simpler to me, but maybe I misunderstand the suggestion.

deads2k · 2016-11-18T19:34:39Z

pkg/authorization/admission/restrictusers/restrictusers.go

+			}
+
+			for _, literal := range userInGroupLiterals {
+				for _, group := range user.Groups {


There should be a groupcache which the API server already maintains to get group information for a user. Use that instead of hitting the API for it.

Oh, I see. You need labels too. nevermind.

deads2k · 2016-11-18T19:38:18Z

Let's the types well documented and then make the SubjectChecker (or a better name if you have it) build for each kind of matcher. That's going to be the easiest thing for api-review to look at to understand behavior.

Also, RoleBindingRestrictions as the overall type?

deads2k · 2016-11-18T19:39:58Z

pkg/authorization/admission/restrictusers/restrictusers.go

+		return nil
+	}
+
+	matchersList, err := q.oclient.CollaboratorMatchers(ns).List(kapi.ListOptions{})


If I have no matchers, seems like I should let anyone bind in. That would be backwards compatible.

Miciah · 2016-11-19T08:23:13Z

I have updated the PR:

• Renamed fields in UserMatcher, GroupMatcher, and ServiceAccountMatcher to be more succinct.
• Made ServiceAccountMatcher into a tuple of serviceaccount name and namespace.
• Copied docstrings from v1/api/types.go into api/types.go.
• Merged pkg/authorization/admission/restrictusers/api/ into pkg/authorization/api/.
• Moved pkg/authorization/admission/restrictusers/registry/collaboratormatcherregistry under pkg/authorization/registry/.
• Changed func (r *restrictUsersAdmission) Admit to use a.GetOldObject() and deleted all obviated informer code.
• Defined a SubjectChecker interface with AddMatcher and Allowed methods.
• Defined a RoleBindingRestrictions type that implements SubjectChecker, and changed RestrictUsersAdmission to use it.
• Updated tests, added some docstrings, probably made some other minor changes.

@deads2k, what do you think now?

deads2k · 2016-11-21T13:06:54Z

pkg/authorization/admission/restrictusers/subjectchecker.go

+// SubjectChecker before use.
+type SubjectChecker interface {
+	Allowed(kapi.ObjectReference) error
+	AddMatcher(authorizationapi.CollaboratorMatcher) error


I don't want this on the interface

deads2k · 2016-11-21T13:07:33Z

pkg/authorization/admission/restrictusers/subjectchecker.go

+// RoleBindingRestrictions aggregates rules specified by CollaboratorMatcher
+// objects in a namespace and uses these rules to determine whether subjects
+// (users, groups, or service accounts) are collaborators on the namespace.
+type RoleBindingRestrictions struct {


I'd like one struct for each kind of Matcher.

deads2k · 2016-11-21T13:08:48Z

pkg/authorization/admission/restrictusers/subjectchecker.go

+// the namespace.
+func (r *RoleBindingRestrictions) Allowed(subject *kapi.ObjectReference) error {
+	switch subject.Kind {
+	case authorizationapi.SystemUserKind:


I'd like to start by managing these inside their respect user and group restrictions, but match them based only on literals.

deads2k · 2016-12-13T21:27:04Z

pkg/authorization/admission/restrictusers/subjectchecker_test.go

+	var (
+		userBobRef = kapi.ObjectReference{
+			Kind:      authorizationapi.UserKind,
+			Namespace: "namespace",


users don't have namespaces.

deads2k · 2016-12-13T21:31:23Z

pkg/authorization/api/v1/register.go

 	)
 	return nil
 }
+
+func (obj *RoleBindingRestrictionList) GetObjectKind() unversioned.ObjectKind { return &obj.TypeMeta }


I expected you to get these automatically. Can you delete them and have it still compile/work?

deads2k · 2016-12-13T21:35:12Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -137,7 +137,7 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				authorizationapi.NewRule(read...).Groups(certificatesGroup).Resources("certificatesigningrequests", "certificatesigningrequests/approval", "certificatesigningrequests/status").RuleOrDie(),

 				authorizationapi.NewRule(read...).Groups(authzGroup).Resources("clusterpolicies", "clusterpolicybindings", "clusterroles", "clusterrolebindings",
-					"policies", "policybindings", "roles", "rolebindings").RuleOrDie(),
+					"policies", "policybindings", "roles", "rolebindings", "rolebindingrestrictions").RuleOrDie(),


a project admin should be allowed to see the rolebindingrestrictions

deads2k · 2016-12-13T21:36:34Z

pkg/cmd/server/origin/master_config.go

@@ -330,6 +331,7 @@ var (
 	// openshiftAdmissionControlPlugins gives the in-order default admission chain for openshift resources.
 	openshiftAdmissionControlPlugins = []string{
 		"ProjectRequestLimit",
+		"openshift.io/RestrictSubjectBindings",


run after originnamespacelifecycle

deads2k · 2016-12-13T21:37:58Z

Comments in a separate commit if you would. Almost done.

Miciah · 2016-12-13T22:52:19Z

I've updated generated files and pushed a new commit addressing the latest review comments.

Miciah · 2016-12-14T01:38:24Z

Saw what could be flake #9681 again:

deploymentconfigs
/data/src/github.com/openshift/origin/test/extended/deployments/deployments.go:845
  with multiple image change triggers [Conformance]
  /data/src/github.com/openshift/origin/test/extended/deployments/deployments.go:335
    should run a successful deployment with multiple triggers [It]
    /data/src/github.com/openshift/origin/test/extended/deployments/deployments.go:326

    Expected error:
        <*errors.errorString | 0xc8200d3780>: {
            s: "timed out waiting for the condition",
        }
        timed out waiting for the condition
    not to have occurred

    /data/src/github.com/openshift/origin/test/extended/deployments/deployments.go:325

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/9550/consoleFull

Miciah · 2016-12-14T05:31:51Z

Flake #12235 (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/9552/consoleFull).

New flake #12251 (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_networking/2380/consoleFull).

Also, fixed the bootstrap policy again. We want to allow admin and cluster-reader to be able to read rolebindingrestrictions, right? With only the former, I get the following error:

--- FAIL: TestClusterReaderCoverage (4.09s)
	authorization_test.go:171: cluster-reader role is missing map[{ rolebindingrestrictions}:true].  Check pkg/cmd/server/bootstrappolicy/policy.go.

deads2k · 2016-12-14T13:25:01Z

pkg/authorization/admission/restrictusers/subjectchecker.go

-			return true, nil
+			errs = append(errs, err)
+		} else if allowed {
+			return true, kerrors.NewAggregate(errs)


return true, nil. We don't want the errors bubbling back since it will make the impl harder in the admission plugin and ultimately they don't matter if we're allowed.

deads2k · 2016-12-14T13:26:36Z

pkg/authorization/admission/restrictusers/restrictusers.go

@@ -157,6 +153,9 @@ func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) {

 		glog.V(4).Infof("Handling policybinding %s/%s",
 			policybinding.Namespace, policybinding.Name)
+
+	default:


duplicate these switch checks at the top of the method, no impl inside of it. That way we get a 4 line check everyone can see:

// we only care about rolebindings and policybindings switch resource{ case rolebindings: case policybindings: default: return nil }

deads2k · 2016-12-14T13:27:42Z

Two minor comments I'd like to see fixed before merge. Go ahead and squash them in and this will be ready.

Add the RestrictUsersAdmission admission plugin for collaboration.

Miciah · 2016-12-14T19:02:51Z

I've made those two changes:
https://github.com/openshift/origin/pull/11743/files#diff-93fae1b66fbece23edf504e5e5aebf1aR42
https://github.com/openshift/origin/pull/11743/files#diff-cbe61dec94c120466223271f21bcd664R88
and squashed.

openshift-bot · 2016-12-14T19:05:32Z

Evaluated for origin test up to 471d717

openshift-bot · 2016-12-14T19:05:41Z

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12390/) (Base Commit: e9e7618)

openshift-bot · 2016-12-14T20:17:16Z

continuous-integration/openshift-jenkins/test SUCCESS

deads2k · 2016-12-14T20:24:00Z

lgtm merge

deads2k · 2016-12-14T20:24:35Z

Disabled the merge tag.

@ncdc Are we good to merge this or are you close enough we should hold it?

ncdc · 2016-12-14T20:33:23Z

I am probably still a few days away.

deads2k · 2016-12-14T21:27:41Z

[merge]

openshift-bot · 2016-12-14T21:29:23Z

Evaluated for origin merge up to 471d717

openshift-bot · 2016-12-15T08:05:27Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12405/) (Base Commit: 46585f4) (Image: devenv-rhel7_5549)

Miciah · 2016-12-15T08:58:50Z

Flake #11016.
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/9597/consoleFull

[networking][router] openshift routers
/data/src/github.com/openshift/origin/test/extended/router/scoped.go:135
  The HAProxy router
  /data/src/github.com/openshift/origin/test/extended/router/scoped.go:134
    should override the route host with a custom value [It]
    /data/src/github.com/openshift/origin/test/extended/router/scoped.go:133

    Expected error:
        <*errors.errorString | 0xc8200f5600>: {
            s: "timed out waiting for the condition",
        }
        timed out waiting for the condition
    not to have occurred

    /data/src/github.com/openshift/origin/test/extended/router/scoped.go:95

Miciah changed the title ~~Initial implementation of RestrictUsersAdmission~~ Introduce RestrictUsersAdmission plugin for collaboration Nov 3, 2016

Miciah mentioned this pull request Nov 3, 2016

describe how restrict which subjects can be bound in a project #9932

Closed

deads2k reviewed Nov 7, 2016

View reviewed changes

Miciah force-pushed the RestrictUsersAdmission branch 3 times, most recently from 90211eb to 9f692fb Compare November 18, 2016 18:53

Miciah commented Nov 18, 2016

View reviewed changes

deads2k reviewed Nov 18, 2016

View reviewed changes

Miciah force-pushed the RestrictUsersAdmission branch from 9f692fb to 21038d5 Compare November 19, 2016 07:43

deads2k reviewed Nov 21, 2016

View reviewed changes

deads2k reviewed Dec 13, 2016

View reviewed changes

Miciah force-pushed the RestrictUsersAdmission branch from 4889e10 to c8b0909 Compare December 13, 2016 22:51

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2016

Miciah force-pushed the RestrictUsersAdmission branch from c8b0909 to 79f03c9 Compare December 13, 2016 23:56

Miciah force-pushed the RestrictUsersAdmission branch from 79f03c9 to 6266fda Compare December 14, 2016 05:32

deads2k reviewed Dec 14, 2016

View reviewed changes

Introduce RestrictUsersAdmission admission plugin

471d717

Add the RestrictUsersAdmission admission plugin for collaboration.

Miciah force-pushed the RestrictUsersAdmission branch from 6266fda to 471d717 Compare December 14, 2016 19:01

Miciah mentioned this pull request Dec 14, 2016

Introduce RestrictUsersAdmission admission plugin #12268

Merged

openshift-bot merged commit b0f45e5 into openshift:master Dec 15, 2016

		@@ -5,7 +5,7 @@ syntax = 'proto2';

		package ytb.ewangdian.workers.dev.openshift.origin.pkg.security.api.v1;

		import "github.com/openshift/origin/pkg/quota/api/v1/generated.proto";

Introduce RestrictUsersAdmission plugin for collaboration #11743

Introduce RestrictUsersAdmission plugin for collaboration #11743

Conversation

Miciah commented Nov 2, 2016 • edited Loading

Miciah commented Nov 3, 2016

Miciah commented Nov 3, 2016

Miciah commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Nov 8, 2016 via email

deads2k commented Nov 8, 2016

Choose a reason for hiding this comment

Miciah commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Nov 18, 2016

Choose a reason for hiding this comment

Miciah commented Nov 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Dec 13, 2016

Miciah commented Dec 13, 2016

Miciah commented Dec 14, 2016

Miciah commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Dec 14, 2016

Miciah commented Dec 14, 2016

openshift-bot commented Dec 14, 2016

openshift-bot commented Dec 14, 2016

openshift-bot commented Dec 14, 2016

deads2k commented Dec 14, 2016 • edited Loading

deads2k commented Dec 14, 2016

ncdc commented Dec 14, 2016

deads2k commented Dec 14, 2016

openshift-bot commented Dec 14, 2016

openshift-bot commented Dec 15, 2016 • edited Loading

Miciah commented Dec 15, 2016

Miciah commented Nov 2, 2016 •

edited

Loading

deads2k commented Dec 14, 2016 •

edited

Loading

openshift-bot commented Dec 15, 2016 •

edited

Loading