feat(backend): Refactor authz to perform SubjectAccessReview. Fixes #3513 #4723

elikatsis · 2020-11-05T19:32:51Z

Description of your changes:

With this PR we implement the refactoring described in #3513.

In the past, KFP used KFAM for its authorization checks, checking only if the user can view the corresponding namespace.
We now refactor the authorization mechanism to perform proper and fine-grained authorization via K8s RBAC and SubjectAccessReview.

Closes #3513

/assign @Bobgy

cc @yanniszark

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

Do you want this pull request (PR) cherry-picked into the current release branch?

Learn more about cherry-picking updates into the release branch.

Bobgy

Thank you so much @elikatsis for the contribution! I have been looking forward for this for a while!

Some early feedback first. I haven't gone through everything.

backend/src/apiserver/client/subject_access_review_fake.go

backend/src/apiserver/client/subject_access_review.go

Bobgy · 2020-11-06T09:23:05Z

@elikatsis I've got one general question, what would be the replacement you think for KFAM for namespace discovery?
Previously, we relied on centraldashboard talking to KFAM to get that, but I believe we can no longer do so if we completely remove KFAM.

I'm thinking about listing all namespaces with a certain pipelines.kubeflow.org/xxx label and build a new selector using this info collected in backend. I wonder what do you think about it.

/cc @yanniszark

elikatsis · 2020-11-09T13:58:19Z

/retest

Bobgy · 2020-11-11T10:32:44Z

I was kind of busy, will review tomorrow

backend/src/apiserver/client/subject_access_review.go

backend/src/apiserver/client/subject_access_review_fake.go

Bobgy

Thank you so much @elikatsis!
I just read through all the code, left some nit pickings. The high-level structure looks awesome to me. Let's follow up with some details and I think we'll be good to go.

Another general nit picking I've seen a few times is that for each resource we defined a canAccessXXX(id) method that takes the ID, so that we needs to first fetch this resource to know its namespace. In cases that we already fetch this resource and know its namespace, we wrote one-off boilerplate code to fetch the resource and check authorization.

I think we can simplify a little bit by defining two utility methods for each resource:

canAccessXxxById(id, verb) --- fetches the resource inside and figures out namespace/name
canAccessXxx(namespace, name, verb) --- we are already fetching the resource, so we can pass namespace and name directly

canAccessXxxById will be straightforward to implement using canAccessXxx and we can avoid quite some duplication there.

Bobgy · 2020-11-12T07:34:07Z

manifests/kustomize/base/pipeline/cluster-scoped/view-edit-roles.yaml

+  clusterRoleSelectors:
+  - matchLabels:
+      rbac.authorization.kubeflow.org/aggregate-to-kubeflow-pipeline-edit: "true"
+rules: []


Why do we separate out actual rules to aggregate-to-pipeline-view ClusterRole?

@Bobgy,
Great question! The answer is this essentially: kubernetes/kubernetes#65171

TL;DR: We can't have both aggregation and rules in a [Cluster]Role. The rules will be ignored. This is why we need to define our rules in some [Cluster]Roles and then have one role to find them, one role to bring them, and in itself bind them 😄

Thanks for the explanation! Good to know. : )

manifests/kustomize/base/pipeline/cluster-scoped/view-edit-roles.yaml

manifests/gcp_marketplace/chart/kubeflow-pipelines/templates/pipeline.yaml

backend/src/apiserver/server/auth_server_test.go

backend/src/apiserver/server/experiment_server.go

backend/src/apiserver/server/util.go

backend/src/apiserver/server/job_server.go

Bobgy · 2020-11-12T09:02:38Z

Other than that, can you resolve the conflicts?

maganaluis · 2020-11-16T03:49:44Z

We are working on #4197 and I just realized our changes will also have conflicts with this PR. Just for my own planning, will this be merged for the release 1.3? What else will we need to integrate with SubjectAccessReview. I know the CentralDashboard also uses KFam.

@elikatsis @Bobgy

Bobgy · 2020-11-16T07:55:16Z

Yes, I think as long as @elikatsis gets through the comments, we can merge.

It should be ready with kubeflow 1.3

This change is compatible with central dashboard, profile controller and kfam, because profile controller should already give service accounts rolebindings that can aggregate ones in this PR.

Bobgy · 2020-11-16T07:57:43Z

It'll be ideal if we can get rid of kfam from central dashboard too, so that we can completely remove it from the manifests, but there are still some unanswered design questions.

elikatsis · 2020-11-16T16:44:28Z

@Bobgy
I rebased and force-pushed. The minor nits (gcp marketplace manifests and forgotten double error check) are included in the "old" commits.
For the comments about

centralized check for multi-user shared mode
use canAccessXXX() instead of isAuthorized()
enhancement on error assertion wrt unauthorized calls
I have added two fixup commits. I will squash them if they look good to you.

As far as having general canAccessXXX() and canAccessXXXById() functions, I think we don't need them. This is covered by each server's methods (see first fixup! commit). Implementing them in a different place and have the server use them looks like it adds complexity instead of simplifying things.

In preparation of SubjectAccessReview, we implement some helpers to create a new Kubernetes Authorization clientset and return the SubjectAccessReview client. We also define some fake clients to be used by future tests.

In preparation of SubjectAccessReview, introduce RBAC groups, resources, and verbs.

Authorization should be based on performing some action on a resource living in a namespace. This commit refactors the authorization utilities to reflect this and perform SubjectAccessReview. This commit also deletes some tests based on old authn/authz mechanism. A following commit will fix/extend the tests for the new mechanism

Bobgy · 2020-11-17T11:01:32Z

backend/src/apiserver/server/util.go

 	}

-	glog.Infof("Authorized user %s in namespace %s", userIdentity, namespace)
+	glog.Infof("Authorized user '%s': %+v", userIdentity, resourceAttributes)


These authorization logs will be very common, every request comes with it.
Shall we change log level to trace or debug?

For most of the cases, users should depend on error message for debugging, rather than api server logs.

I can only see Info, Warning, Error, and Fatal levels. Can you help me how can I do debug or sth different?
Here's what I found in the repo, not sure if it helps: https://github.com/golang/glog/blob/23def4e6c14b4da8ac2ed8007337bc5eb5007998/glog.go#L91-L115

OK, sorry I thought there were more. Then let's go with info for now.

manifests/kustomize/base/pipeline/multi-user/view-edit-roles.yaml

Bobgy

I've only got two comments left. Everything else looks great to me!
Thank you so much @elikatsis!

/lgtm

With KFAM authorization, we passed only the namespace attribute for authorization. With SubjectAccessReview, we need a richer list of attributes. Thus, we adjust endpoints to pass request details (resource attributes) necessary for authorizing the request. We only change the already authorized endpoints, not introducing any new checks.

Since we no longer use KFAM, we may as well purge it

Signed-off-by: Ilias Katsakioris <[email protected]>

* API Server: Allow creating SubjectAccessReviews * Add view/edit roles in a multi-user kustomization

elikatsis · 2020-11-17T15:44:51Z

@Bobgy I added the comment and squashed the fixups. If you can help me selecting the logging level, we'll be golden!

Bobgy · 2020-11-17T15:51:50Z

Do you need @yanniszark 's review?
Everything good on my side

elikatsis · 2020-11-17T16:13:30Z

@Bobgy we have already reviewed this internally, so we should be good to go. Thank you very much for your comments! 😃

Bobgy · 2020-11-17T22:54:50Z

/lgtm
/approve
Cool, I am feeling excited about the huge potential this brings. Thank you again for this great contribution!

k8s-ci-robot · 2020-11-17T22:54:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, elikatsis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~backend/OWNERS~~ [Bobgy]
~~manifests/kustomize/OWNERS~~ [Bobgy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Update apiserver role to allow creating SubjectAccessReviews * Add roles which get aggregated to kubeflow-view and kubeflow-edit Original PR: kubeflow/pipelines#4723 Signed-off-by: Ilias Katsakioris <[email protected]>

…ubeflow#3513 (kubeflow#4723) * [Backend] Return proper error codes for failures during auth * [Backend] Implement helpers to initialize a SubjectAccessReview client In preparation of SubjectAccessReview, we implement some helpers to create a new Kubernetes Authorization clientset and return the SubjectAccessReview client. We also define some fake clients to be used by future tests. * [Backend] Introduce RBAC-related constants In preparation of SubjectAccessReview, introduce RBAC groups, resources, and verbs. * [Backend] Extend managers with a SubjectAccessReviewClient * [Backend] Refactor the authorization mechanism for requests Authorization should be based on performing some action on a resource living in a namespace. This commit refactors the authorization utilities to reflect this and perform SubjectAccessReview. This commit also deletes some tests based on old authn/authz mechanism. A following commit will fix/extend the tests for the new mechanism * [Backend] Adjust endpoints to pass resource attributes for authz With KFAM authorization, we passed only the namespace attribute for authorization. With SubjectAccessReview, we need a richer list of attributes. Thus, we adjust endpoints to pass request details (resource attributes) necessary for authorizing the request. We only change the already authorized endpoints, not introducing any new checks. * [Backend] Adjust apiserver/server tests to SubjectAccessReview * [Backend] Purge KFAM Since we no longer use KFAM, we may as well purge it * [Backend] Update BUILD files Signed-off-by: Ilias Katsakioris <[email protected]> * [Manifests] Extend manifests for SubjectAccessReview * API Server: Allow creating SubjectAccessReviews * Add view/edit roles in a multi-user kustomization

k8s-ci-robot assigned Bobgy Nov 5, 2020

k8s-ci-robot requested review from Bobgy and rmgogogo November 5, 2020 19:32

k8s-ci-robot added the size/XXL label Nov 5, 2020

google-cla bot added the cla: yes label Nov 5, 2020

Bobgy reviewed Nov 5, 2020

View reviewed changes

Bobgy mentioned this pull request Nov 6, 2020

support separate pipeline for each namespace #4197

Open

k8s-ci-robot requested a review from yanniszark November 6, 2020 09:23

elikatsis force-pushed the feature-subject-access-review branch from 36c0eab to b570153 Compare November 6, 2020 10:57

maganaluis mentioned this pull request Nov 9, 2020

kfam/v1/bindings taking over 8 seconds kubeflow/kubeflow#5377

Closed

elikatsis mentioned this pull request Nov 9, 2020

Enable running the apiserver locally #4738

Closed

Bobgy reviewed Nov 12, 2020

View reviewed changes

backend/src/apiserver/client/subject_access_review.go Outdated Show resolved Hide resolved

backend/src/apiserver/client/subject_access_review_fake.go Outdated Show resolved Hide resolved

Bobgy reviewed Nov 12, 2020

View reviewed changes

[Backend] Return proper error codes for failures during auth

327d168

elikatsis force-pushed the feature-subject-access-review branch 2 times, most recently from 29827e3 to a19543e Compare November 16, 2020 16:41

elikatsis force-pushed the feature-subject-access-review branch from a19543e to 480adcd Compare November 16, 2020 17:33

elikatsis added 2 commits November 16, 2020 20:02

[Backend] Introduce RBAC-related constants

b80b478

In preparation of SubjectAccessReview, introduce RBAC groups, resources, and verbs.

[Backend] Extend managers with a SubjectAccessReviewClient

7d62f5c

elikatsis force-pushed the feature-subject-access-review branch 2 times, most recently from a3a7abd to 9323c40 Compare November 16, 2020 18:12

elikatsis force-pushed the feature-subject-access-review branch from 9323c40 to 53d4e9a Compare November 16, 2020 18:17

Bobgy reviewed Nov 17, 2020

View reviewed changes

manifests/kustomize/base/pipeline/multi-user/view-edit-roles.yaml Show resolved Hide resolved

Bobgy reviewed Nov 17, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Nov 17, 2020

elikatsis added 5 commits November 17, 2020 17:39

[Backend] Adjust apiserver/server tests to SubjectAccessReview

d21d5fe

[Backend] Purge KFAM

68176ca

Since we no longer use KFAM, we may as well purge it

[Backend] Update BUILD files

762639a

Signed-off-by: Ilias Katsakioris <[email protected]>

[Manifests] Extend manifests for SubjectAccessReview

6302735

* API Server: Allow creating SubjectAccessReviews * Add view/edit roles in a multi-user kustomization

elikatsis force-pushed the feature-subject-access-review branch from 53d4e9a to 6302735 Compare November 17, 2020 15:44

k8s-ci-robot removed the lgtm label Nov 17, 2020

k8s-ci-robot added the lgtm label Nov 17, 2020

k8s-ci-robot added the approved label Nov 17, 2020

k8s-ci-robot merged commit 39203d5 into kubeflow:master Nov 17, 2020

elikatsis mentioned this pull request Nov 18, 2020

Update manifests wrt kubeflow/pipelines#4723 kubeflow/manifests#1659

Closed

elikatsis deleted the feature-subject-access-review branch November 23, 2020 12:20

davidspek mentioned this pull request Dec 23, 2020

[Multi User] failed to call kfp.Client().create_run_from_pipeline_func in in-cluster juypter notebook #4440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): Refactor authz to perform SubjectAccessReview. Fixes #3513 #4723

feat(backend): Refactor authz to perform SubjectAccessReview. Fixes #3513 #4723

elikatsis commented Nov 5, 2020

Bobgy left a comment

Bobgy commented Nov 6, 2020

elikatsis commented Nov 9, 2020

Bobgy commented Nov 11, 2020

Bobgy left a comment

Bobgy Nov 12, 2020

elikatsis Nov 16, 2020

Bobgy Nov 17, 2020

Bobgy commented Nov 12, 2020

maganaluis commented Nov 16, 2020

Bobgy commented Nov 16, 2020

Bobgy commented Nov 16, 2020

elikatsis commented Nov 16, 2020

Bobgy Nov 17, 2020

elikatsis Nov 17, 2020

Bobgy Nov 17, 2020

Bobgy left a comment

elikatsis commented Nov 17, 2020

Bobgy commented Nov 17, 2020

elikatsis commented Nov 17, 2020

Bobgy commented Nov 17, 2020

k8s-ci-robot commented Nov 17, 2020

feat(backend): Refactor authz to perform SubjectAccessReview. Fixes #3513 #4723

feat(backend): Refactor authz to perform SubjectAccessReview. Fixes #3513 #4723

Conversation

elikatsis commented Nov 5, 2020

Bobgy left a comment

Choose a reason for hiding this comment

Bobgy commented Nov 6, 2020

elikatsis commented Nov 9, 2020

Bobgy commented Nov 11, 2020

Bobgy left a comment

Choose a reason for hiding this comment

Bobgy Nov 12, 2020

Choose a reason for hiding this comment

elikatsis Nov 16, 2020

Choose a reason for hiding this comment

Bobgy Nov 17, 2020

Choose a reason for hiding this comment

Bobgy commented Nov 12, 2020

maganaluis commented Nov 16, 2020

Bobgy commented Nov 16, 2020

Bobgy commented Nov 16, 2020

elikatsis commented Nov 16, 2020

Bobgy Nov 17, 2020

Choose a reason for hiding this comment

elikatsis Nov 17, 2020

Choose a reason for hiding this comment

Bobgy Nov 17, 2020

Choose a reason for hiding this comment

Bobgy left a comment

Choose a reason for hiding this comment

elikatsis commented Nov 17, 2020

Bobgy commented Nov 17, 2020

elikatsis commented Nov 17, 2020

Bobgy commented Nov 17, 2020

k8s-ci-robot commented Nov 17, 2020