User Impersonation Implementation #428

shawn-hurley · 2017-09-08T18:12:45Z

Describe what this PR does and why we need it:
This PR will allow us to check for users ability to run the APB, will run APB in transient name space that will hide the escalated permissions.
Changes proposed in this pull request

Add open shift client and create/delete project, subject rules review ability
Add copied security/validation code from origin to check if rules cover the cluster role's rules .
Add ability to pull out originating user header info.

Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on
kubernetes-retired/service-catalog#1162

Note:

This PR will depend on using openshift 3.7

shawn-hurley · 2017-09-08T20:02:38Z

(5) Add Originating Identity Header to the broker [asb]

shawn-hurley · 2017-09-08T20:02:54Z

(13) Service Account privileges for APBs and the Broker

shawn-hurley · 2017-09-08T20:15:00Z

To Test

curl -s -k -XPUT -u admin:admin -H "Content-Type: application/json" -H "X-Broker-API-Originating-Identity: kubernetes eyJ1c2VybmFtZSI6ICJhZG1pbiIsICJ1aWQiOiAiIn0=" -d '{"service_id":"4b058efd9ed2414e6cfdee4bda167e9a","plan_id":"default","organization_guid":"8a7d0e88-94a7-11e7-8039-c85b76064d75","space_guid":"8a7d0e88-94a7-11e7-8039-c85b76064d75","parameters":{"postgresql_database":"admin","postgresql_password":"admin","postgresql_user":"admin"},"context":{"namespace":"demo-app","platform":"kubernetes"}}' https://asb-1338-ansible-service-broker.172.17.0.1.nip.io/v2/service_instances/97f2b6c3-45ba-4e6c-a372-77c43fa0fc9e?accepts_incomplete=true

Will use the admin user for the user info.

You will also need to specifically give asb access to create and delete. This will be added to the template in this PR. but example of the cluster role is

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1beta1
metadata:
# "namespace" omitted since ClusterRoles are not namespaced
  name: project-creator
rules:
- apiGroups: ["project.openshift.io"]
  resources: ["projects"]
  verbs: ["create", "delete"]
- apiGroups: ["authorization.openshift.io"]
  resources: ["subjectrulesreview"]
  verbs: ["create"]

and cluster role binding

kind: ClusterRoleBinding
apiVersion: rbac.authorization.k8s.io/v1beta1
metadata:
  name: create-projects-asb
subjects:
- kind: ServiceAccount 
  name: asb
  namespace: ansible-service-broker
roleRef:
  kind: ClusterRole
  name: project-creator 
  apiGroup: rbac.authorization.k8s.io

shawn-hurley · 2017-09-11T12:31:32Z

This will need to be run with 3.7. It is the only way that this will work.

enj · 2017-09-11T15:58:09Z

@openshift/sig-security

It might be a bit before I have a chance to look at this.

shawn-hurley · 2017-09-12T12:47:27Z

@enj The reason for the v1 apiVersion is that it works for both 3.6 and 3.7. If you have a better way to do it and keep compatibility with 3.6 It would be nice to know and we can update all of the v1 references in the template.

liggitt · 2017-09-12T13:16:48Z

The reason for the v1 apiVersion is that it works for both 3.6 and 3.7. If you have a better way to do it and keep compatibility with 3.6 It would be nice to know and we can update all of the v1 references in the template.

At the very least, use the grouped version of openshift APIs, e.g. authorization.openshift.io/v1

shawn-hurley · 2017-09-12T13:45:47Z

@liggitt @enj I have updated the template to use the grouped version of the openshift APIs. @jwmatthews this still works with the 3.6 version of catasb

jmrodri

The github UI is unbearable for this review. I just cloned the repo and looked at it through tig instead. I didn't see anything glaring, the glide.yaml will be different than what I have for my work, but I can deal with that when the time comes. All in all, great work.

shawn-hurley · 2017-09-12T15:50:18Z

Added #429 to capture moving all of the template yaml to RBAC API versions when we make the cut to 3.7.

shawn-hurley · 2017-09-20T20:31:25Z

pkg/clients/openshift.go

+func (o OpenshiftClient) SubjectRulesReview(user, namespace string, log *logging.Logger) (result *authorization.SubjectRulesReview, err error) {
+	body := &SubjectRulesReview{
+		Spec: SubjectRulesReviewSpec{
+			User: "admin",


change to user

jwmatthews · 2017-09-21T13:20:15Z

Service Catalog 0.0.21 will send the Originating User header to us.

Full end to end testing of this PR is waiting for below to merge into origin:
openshift/origin#16457

shawn-hurley · 2017-09-21T16:15:05Z

pkg/apb/executor.go

+		// Create namespace.
+		namespace := v1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{transientNameSpaceKey: spec.FQName},


@enj creating namespaces and adding the metadata about which APB started the namespace.

shawn-hurley · 2017-09-21T16:15:56Z

pkg/app/app.go

-	err = authorization.ConvertRBACClusterRoleToAuthorizationClusterRole(k8sRole, cRole, nil)
-	if err != nil {
+	rbacClusterRole := rbac.ClusterRole{}
+	if v1beta1rbac.Convert_v1beta1_ClusterRole_To_rbac_ClusterRole(k8sRole, &rbacClusterRole, nil); err != nil {


@enj Getting v1beta1 RBAC rules to RBAC rules.

shawn-hurley · 2017-09-21T16:17:31Z

pkg/clients/openshift.go

-	result.Kind = r.Kind
-	result.APIVersion = r.APIVersion
-	return
+	return authorization.ConvertAPIPolicyRulesToRBACPolicyRules(pr), nil


@enj Converting API Policy Rules to RBAC Policy rules

shawn-hurley · 2017-09-21T16:19:48Z

pkg/handler/handler.go

 	if err != nil {
 		return false, http.StatusInternalServerError, fmt.Errorf("Unable to connect to the cluster")
 	}
-	if covered, _ := authorization.Covers(res.Status.Rules, h.clusterRoleRules); !covered {
+	if covered, _ := validation.Covers(prs, h.clusterRoleRules); !covered {


@enj Using k8s RBAC validation

shawn-hurley · 2017-09-21T16:20:32Z

templates/deploy-ansible-service-broker.template.yaml

@@ -32,10 +32,10 @@ objects:
  apiVersion: authorization.openshift.io/v1
  metadata:
    # "namespace" omitted since ClusterRoles are not namespaced
-    name: project-creator
+    name: asb-auth


@enj Renamed role to something that makes a little more sense

enj · 2017-09-22T19:15:02Z

LGTM

There are some followup items but this is in line with our discussions.

* Using 1.7 request context to pass user info to handler func. * Caching retrieved Cluster Role Rules in the handler. * refactor the cover code to send back errors before async provision can occur * making tests work

Adding back tests by auto escalating user. Will validate the user info from context works. Updating configuration

adding apb check for each call that could launch an APB.

* We must give asb access to subjectrulesreview as well as create projects.

…ror. * can override by specifing auto_escalate to true.

…o true * adding ability for template to set the auto_escalate feature * setting to true so that 3.6 works out of the box.

* moving all policy rules and cover check to kubernetes rbac. * remove project creation and deletion * adding ability to create namespaces instead of projects. * adding labels for metadata

eriknelson

Only a few small nits. Nice job!

eriknelson · 2017-09-27T15:09:21Z

templates/deploy-ansible-service-broker.template.yaml

@@ -27,7 +27,7 @@ objects:
  metadata:
    name: asb
    namespace: ansible-service-broker
-
+    


Can we strip trailing whitespace in this file?

eriknelson · 2017-09-27T15:12:11Z

pkg/clients/openshift.go

+
+// Openshift - Create a new openshift client if needed, returns reference
+func Openshift(log *logging.Logger) (*OpenshiftClient, error) {
+	errMsg := "Something went wrong while initializing kubernetes client!\n"


s/kubernetes/openshift/

eriknelson · 2017-09-27T15:12:28Z

pkg/clients/openshift.go

+		instances.Openshift = client
+	})
+	if instances.Openshift == nil {
+		return nil, errors.New("Kubernetes client instance is nil")


s/Kubernetes/Openshift

rthallisey · 2017-09-27T16:49:48Z

pkg/apb/svc_acct.go

@@ -219,30 +222,24 @@ func (s *ServiceAccountManager) createFile(handle string) (string, error) {
 }

 // DestroyApbSandbox - Destroys the apb sandbox
-func (s *ServiceAccountManager) DestroyApbSandbox(executionContext ExecutionContext) error {
+func (s *ServiceAccountManager) DestroyApbSandbox(executionContext ExecutionContext, clusterConfig ClusterConfig) error {


Instead of passing in the ClusterConfig object just pass in the namespace.

rthallisey · 2017-09-27T17:05:25Z

pkg/clients/openshift.go

+}
+
+// SubjectRulesReview is a resource you can create to determine which actions another user can perform in a namespace
+type SubjectRulesReview struct {


Would prefer all the types in a types.go

We'll move away from types.go. Ignore this comment

* adding ability for executor to create a project * adding openshift client * attempting to add neccessary orgin code to test users rules * working origin authorizer * adding ability to get user info from header. * Adding retrieval of Originating user from header * Using 1.7 request context to pass user info to handler func. * Caching retrieved Cluster Role Rules in the handler. * refactor the cover code to send back errors before async provision can occur * making tests work * Adding auto escalate configuration value. Adding back tests by auto escalating user. Will validate the user info from context works. Updating configuration * Moving handler code to it's own handler. * more refactoring to make it cleaner adding apb check for each call that could launch an APB. * fixing bugs * adding ability to delete project and clean up. * re-implement correct error handling for service instance not found * Updating handler log statements so they are not as bad * Updating deploy template to account for the new cluster role. * We must give asb access to subjectrulesreview as well as create projects. * Checking if cluster roles are retrievable otherwise throw 3.7 only error. * can override by specifing auto_escalate to true. * adding ability for old service catalog to work w/ auto_escalate set to true * adding ability for template to set the auto_escalate feature * setting to true so that 3.6 works out of the box. * Fixing golint errors for the openshift client * addressing PR Review from mo * moving all policy rules and cover check to kubernetes rbac. * remove project creation and deletion * adding ability to create namespaces instead of projects. * adding labels for metadata * fixing go lint issues * changing template to a temp lastest template * rebase and fixing build * update bassed on pr comments * updating based on review comments

shawn-hurley force-pushed the sec-user-imprs branch 2 times, most recently from b528b33 to ded8da9 Compare September 8, 2017 19:24

shawn-hurley changed the title ~~[WIP] User Impersonation Implementation~~ User Impersonation Implementation Sep 8, 2017

shawn-hurley changed the title ~~User Impersonation Implementation~~ [WIP] User Impersonation Implementation Sep 8, 2017

shawn-hurley added the feature label Sep 9, 2017

shawn-hurley changed the title ~~[WIP] User Impersonation Implementation~~ User Impersonation Implementation Sep 11, 2017

shawn-hurley force-pushed the sec-user-imprs branch from 4baea70 to 7028ad1 Compare September 12, 2017 13:44

jmrodri approved these changes Sep 12, 2017

View reviewed changes

shawn-hurley force-pushed the sec-user-imprs branch from 7028ad1 to 2fcbbbd Compare September 12, 2017 15:54

This was referenced Sep 12, 2017

Adding ability to override auto_escalate for a broker fusor/catasb#135

Merged

first pass at administration documentation #430

Merged

shawn-hurley force-pushed the sec-user-imprs branch from bce5c9c to 0cdfd17 Compare September 14, 2017 19:18

shawn-hurley added the needs-upstream label Sep 15, 2017

shawn-hurley commented Sep 20, 2017

View reviewed changes

shawn-hurley force-pushed the sec-user-imprs branch from 0cdfd17 to eb0aa1f Compare September 21, 2017 16:14

shawn-hurley commented Sep 21, 2017

View reviewed changes

shawn-hurley force-pushed the sec-user-imprs branch from 62e80a8 to c8be8d4 Compare September 22, 2017 18:56

shawn-hurley removed the needs-upstream label Sep 25, 2017

shawn-hurley added 18 commits September 26, 2017 13:49

working origin authorizer

2c4539d

adding ability to get user info from header.

2a86105

Adding retrieval of Originating user from header

34ccdae

* Using 1.7 request context to pass user info to handler func. * Caching retrieved Cluster Role Rules in the handler. * refactor the cover code to send back errors before async provision can occur * making tests work

Adding auto escalate configuration value.

7ec111e

Adding back tests by auto escalating user. Will validate the user info from context works. Updating configuration

Moving handler code to it's own handler.

5943f1a

more refactoring to make it cleaner

d92495f

adding apb check for each call that could launch an APB.

fixing bugs

3f10eae

adding ability to delete project and clean up.

8ab590c

re-implement correct error handling for service instance not found

1d20638

Updating handler log statements so they are not as bad

ae72e3b

Updating deploy template to account for the new cluster role.

e51d7cc

* We must give asb access to subjectrulesreview as well as create projects.

Checking if cluster roles are retrievable otherwise throw 3.7 only er…

a86a90b

…ror. * can override by specifing auto_escalate to true.

adding ability for old service catalog to work w/ auto_escalate set t…

228cdc1

…o true * adding ability for template to set the auto_escalate feature * setting to true so that 3.6 works out of the box.

Fixing golint errors for the openshift client

1db73aa

addressing PR Review from mo

5cfc876

* moving all policy rules and cover check to kubernetes rbac. * remove project creation and deletion * adding ability to create namespaces instead of projects. * adding labels for metadata

fixing go lint issues

f064640

changing template to a temp lastest template

5ec4136

rebase and fixing build

84086d9

shawn-hurley force-pushed the sec-user-imprs branch from 74fdcbf to 84086d9 Compare September 26, 2017 20:17

jmrodri requested review from eriknelson and rthallisey September 27, 2017 13:13

eriknelson approved these changes Sep 27, 2017

View reviewed changes

update bassed on pr comments

7b8e456

rthallisey suggested changes Sep 27, 2017

View reviewed changes

updating based on review comments

963d1ff

rthallisey approved these changes Sep 27, 2017

View reviewed changes

shawn-hurley merged commit 0934ce3 into openshift:master Sep 27, 2017

This was referenced Oct 2, 2017

Possibility to run the apb pod at the different namespace than the target namespace #408

Closed

Broker should atomically create ApbSandboxes, and error out if that creation fails. #400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User Impersonation Implementation #428

User Impersonation Implementation #428

shawn-hurley commented Sep 8, 2017 •

edited

shawn-hurley commented Sep 8, 2017

shawn-hurley commented Sep 8, 2017

shawn-hurley commented Sep 8, 2017

shawn-hurley commented Sep 11, 2017

enj commented Sep 11, 2017

shawn-hurley commented Sep 12, 2017

liggitt commented Sep 12, 2017

shawn-hurley commented Sep 12, 2017

jmrodri left a comment

shawn-hurley commented Sep 12, 2017

shawn-hurley Sep 20, 2017

jwmatthews commented Sep 21, 2017

shawn-hurley Sep 21, 2017

shawn-hurley Sep 21, 2017

shawn-hurley Sep 21, 2017

shawn-hurley Sep 21, 2017

shawn-hurley Sep 21, 2017

enj commented Sep 22, 2017

eriknelson left a comment

eriknelson Sep 27, 2017

eriknelson Sep 27, 2017

eriknelson Sep 27, 2017

rthallisey Sep 27, 2017

rthallisey Sep 27, 2017

rthallisey Sep 27, 2017

User Impersonation Implementation #428

User Impersonation Implementation #428

Conversation

shawn-hurley commented Sep 8, 2017 • edited

Note:

shawn-hurley commented Sep 8, 2017

shawn-hurley commented Sep 8, 2017

shawn-hurley commented Sep 8, 2017

To Test

shawn-hurley commented Sep 11, 2017

enj commented Sep 11, 2017

shawn-hurley commented Sep 12, 2017

liggitt commented Sep 12, 2017

shawn-hurley commented Sep 12, 2017

jmrodri left a comment

Choose a reason for hiding this comment

shawn-hurley commented Sep 12, 2017

Choose a reason for hiding this comment

jwmatthews commented Sep 21, 2017

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

enj commented Sep 22, 2017

eriknelson left a 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

shawn-hurley commented Sep 8, 2017 •

edited