Add authorization mechanisms in new Katib UI backend #1983

apo-ger · 2022-10-25T11:47:43Z

What this PR does / why we need it:
The current backend for the new UI does not perform any authorization checks. It should comply with the authz guidelines (kubeflow/kubeflow#5566) and use SubjectAccessReviews (SAR) to check if a user has permissions to perform an action.

This PR:

introduces authz checks for actions on:
- Trials
- Experiments
- TrialTemplates
introduces three ENV variables:
- APP_DISABLE_AUTH: Skip authorization if an admin is explicitly requested it.
- BACKEND_MODE: dev or prod mode. Skip authorization when in dev mode.
- USERID_HEADER: to check and ensure that a username is provided into the header of every http request towards the api-server.

Before any request proceed to K8s api-server:

check if authorization must be skipped (BACKEND_MODE, APP_DISBLE_AUTH)
check if a username is proviced in request Header (authentication)
query the K8s api-server with SAR to ensure that the user has appropriate access privileges

Other changes:

Introduce a client-go client to construct SubjectAccessReview objects.
Replace the /katib/fetch_experiment/ route with /katib/fetch_namespaced_experiments. This route expects a namespace as a query parameter from which all experiments will be fetched.
Update the README of the web app to expose that devs should set APP_DISABLE_AUTH=true when running locally

Fixes #1547

* Introduce helper ENV vars and functions for authentication and authorization checks. The authz checks are using SubjectAcessReviews objects. * BACKEND_MODE={dev,prod}: skip authz when in dev mode * APP_DISABLE_AUTH={bool}: skip authz if explicity requested * Introduce a client-go client to construct SubjectAccessReview objects. * Before any request proceed to K8s api-server: * check if authorization must be skipped (BACKEND_MODE, APP_DISBLE_AUTH) * check if a username is proviced in request Header * query the K8s api-server with SAR to ensure that the user has appropriate access privilleges * Replace the /katib/fetch_experiment/ route with /katib/fetch_namespaces_experiments. This route expects a namespace as a query parameter from which all experiments will be fetched. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

johnugeorge · 2022-10-25T18:57:54Z

/assign @kimwnasptd

Can you review this PR for both standalone and Kubeflow mode of deployment?

andreyvelich · 2022-10-25T19:06:44Z

Thank you so much for these improvements @apo-ger!
Before merging this please can we have discussion at AutoML WG Community Meeting on November 16th ?
In the meantime, @kimwnasptd you can start reviewing this.

cc @akshaychitneni

andreyvelich · 2022-10-25T19:07:31Z

/hold for the review

coveralls · 2022-10-26T03:54:13Z

Coverage increased (+0.07%) to 73.533% when pulling ad1e3b0 on apo-ger:feature-arrikto-apoger-katib-authnz-sar into b3c3807 on kubeflow:master.

apo-ger · 2022-10-31T07:45:46Z

@andreyvelich
Sure, we'll be more than happy to join and go over the details!

apo-ger · 2022-11-16T16:30:42Z

During the review, we took another look at the kubeflow-katib installation manifests and it appears that Istio sidecar injection is currently disabled for the katib-ui component (ui.yaml). The same applies to the other katib components. In addition, we lack the proper AuthorizationPolicies for this PR to work properly, as currently any workloda/pod can set the kubeflow-userid header and communicate with the backend of the Katib UI.

I can prepare a follow-up manifests PR to address this. Minimum required changes:

introduce a patch in kubeflow-katib installation manifests to inject an istio sidecar in katib-ui Pod.
introduce an AuthorizationPolicy to secure katib-ui and allow traffic only from istio-ingressgateway workload. (authenticated requests from the UI with the kubeflow-userid header set)

I could also prepare another PR, which would allow the injection of istio sidecar for the other components of katib and introduce appropriate AuthorizationPolicies to secure them. However, this is orthogonal to this PR.

cc. @kimwnasptd, @andreyvelich, @johnugeorge.

johnugeorge · 2022-11-16T17:55:21Z

Looks good @apo-ger

apo-ger · 2022-11-17T10:24:48Z

pkg/new-ui/v1beta1/backend.go

+		return
+	}
+
+	newTemplates, err := k.updateTrialTemplates(updatedConfigMapNamespace, updatedConfigMapName, "", updatedConfigMapPath, "", ActionTypeDelete, user)


@kimwnasptd Currently the updateTrialTemplate() function is called from multiple routes (Edit/Add/DeleteTemplate). It will first try to retrieve the TrialTemplate ConfigMap and then perform update/delete/create depending on the route from which it is called.

The current implementation will either delete the TrialTemplate ConfigMap if its empty or update the ConfigMap when the actiontype is delete. Currently we check only if a user has permissions to delete ConfigMaps.

We could move the authorization checks inside updateTrialTemplate() since it is called from multiple routes.

tenzen-y

@apo-ger Thanks for driving this PR!
I left a few comments.

pkg/new-ui/v1beta1/authzn.go

pkg/new-ui/v1beta1/hp.go

pkg/new-ui/v1beta1/backend.go

apo-ger · 2022-11-17T11:59:49Z

@tenzen-y
Thank you for the comments! Fixes are now included.

andreyvelich · 2022-11-17T12:43:53Z

During the review, we took another look at the kubeflow-katib installation manifests and it appears that Istio sidecar injection is currently disabled for the katib-ui component (ui.yaml).

@apo-ger Should we remove this annotation from our manifests ? I guess, Katib UI should work with istio sidecar injected, right ?
What about our DB Manager:

katib/manifests/v1beta1/components/db-manager/db-manager.yaml

Line 19 in cdd0b90

sidecar.istio.io/inject: "false"

? Will gRPC communication works with injected sidecar ?

andreyvelich

I left few comments.

cmd/new-ui/v1beta1/main.go

pkg/new-ui/v1beta1/authzn.go

pkg/new-ui/v1beta1/backend.go

pkg/new-ui/v1beta1/authzn.go

pkg/new-ui/v1beta1/backend.go

apo-ger · 2022-11-18T08:09:54Z

@andreyvelich

@apo-ger Should we remove this annotation from our manifests ? I guess, Katib UI should work with istio sidecar injected, right ?

Yes, removing the annotation will do.

What about our DB Manager:

katib/manifests/v1beta1/components/db-manager/db-manager.yaml

Line 19 in cdd0b90

sidecar.istio.io/inject: "false"

? Will gRPC communication works with injected sidecar ?

Injecting a sidecar in DB manager means we need to introduce an additional AuthorizationPolicy to explicitly allow traffic from other katib components (katib-ui, katib-controller etc) when in kubeflow mode as by default we deny all traffic in the mesh: https://github.com/kubeflow/manifests/blob/master/common/istio-1-14/istio-install/base/deny_all_authorizationpolicy.yaml.

This is needed for the new /katib/fetch_namespaced_experiments route. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

Update the README of the web app to expose that devs should set APP_DISABLE_AUTH=true when running locally, since there's no authnz when running locally. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

* proper error handling. * switch to Go's build-in errors package. * set appropriate verbs when constructing SAR objects. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

* fix backend routes. * '/katib/fetch_namespaces' to fetch experiments in a namespace * 'FetchExperiments' handler * hit the appropriate route from frontend and provide namespace as a query parameter to fetch experiments * remove remove BACKEND_MODE env var in favour of the more specific APP_DISABLE_AUTH Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

apo-ger · 2022-11-18T15:28:36Z

@andreyvelich Thank you for your comments. Please let me know if everything is okay

Now, regarding the manifests changes:

IIUC I should enable istio sidecar injection for both katib-ui and DB manager pods, right?
- by removing the annotation:
  
  katib/manifests/v1beta1/components/db-manager/db-manager.yaml
  
  Line 19 in cdd0b90
  
  sidecar.istio.io/inject: "false"
I'll create a follow-up PR as soon as this PR is merged.

andreyvelich · 2022-11-18T15:33:05Z

@apo-ger I am not sure if we should enable istio for DB Manager. Will it block gRPC traffic that call APIs:
Experiment Controller <-> Suggestion Service, Trial Metrics Collector <-> Katib DB Manager ?

cc @johnugeorge @kimwnasptd @tenzen-y

pkg/new-ui/v1beta1/authzn.go

pkg/new-ui/v1beta1/backend.go

andreyvelich · 2022-11-18T15:46:39Z

pkg/new-ui/v1beta1/authzn.go

+	if DISABLE_AUTH == "true" {
+		log.Printf("APP_DISABLE_AUTH set to True. Skipping authentication check")
+		return "", nil
+	}


Can we avoid duplicate check for if DISABLE_AUTH == "true" by adding GetUsername logic under IsAuthorized call ?
Then, we have 1 place where we verify if auth is required.
WDYT @apo-ger ?

Sure, I moved the GetUsername logic under IsAuthorized and introduced the appropriate checks to handle the various errors properly!

pkg/new-ui/v1beta1/backend.go

pkg/new-ui/v1beta1/hp.go

pkg/new-ui/v1beta1/util.go

johnugeorge · 2022-11-18T16:16:04Z

@apo-ger I am not sure if we should enable istio for DB Manager. Will it block gRPC traffic that call APIs: Experiment Controller <-> Suggestion Service, Trial Metrics Collector <-> Katib DB Manager ?

cc @johnugeorge @kimwnasptd @tenzen-y

Note: We have to test it carefully before any side car changes. The reason is that we don't have any tests in Kubeflow mode. So, manual testing is the only option

andreyvelich · 2022-11-21T13:20:56Z

pkg/new-ui/v1beta1/util.go

+				user, err := IsAuthorized(consts.ActionTypeGet, ns, apiv1.ResourceConfigMaps.String(), "", configmapName, apiv1.SchemeGroupVersion, k.katibClient.GetClient(), r)
+				if err != nil {
+					log.Printf("The user: %s is not authorized to view configMap: %s in namespace: %s \n", user, configmapName, ns)
+					return nil, err
+				} else {
+					log.Printf("The user: %s is authorized to view configMap: %s in namespace: %s", user, configmapName, ns)
+					newTrialTemplatesConfigMapList = append(newTrialTemplatesConfigMapList, cmap)
+				}


@apo-ger Maybe we should make the IsAuthorized check before we are getting Trial templates ?
Then, you don't need to have additional condition for kubeflow namespace.

Okay, let me explain the rationale behind this particular check.

Right now all the TrialTemplates reside in the kubeflow namespace. I understand that this might change to have TrialTemplates per namespace at some point (#1546)

Since this is not the case right now, we decided to allow all users to view the default TrialTemplates in the kubeflow namespace and this is why we perform the authorization checks after getting the Trial Templates. The current implementation:

fetches the trialTemplate ConfigMaps from all namespaces

constructs a trialTemplatesConfigMapList list which includes:

the default TrialTemplate ConfigMap that belong to the kubeflow namespace

the ConfigMaps which belong to namespaces that the user has access rights to (e.g kubeflow-user)

All other CMs are filtered out.

Note that the current implementation ensures that users are able to delete/edit TrialTemplates only in their namespace and not the default ones in the kubeflow ns.

WDYT @andreyvelich ?

IIUC the current implementation of the UI only allows users to view the available TrialTemplates (/katib/fetch_trial_templates/), right?

I see, makes sense @apo-ger.

IIUC the current implementation of the UI only allows users to view the available TrialTemplates (/katib/fetch_trial_templates/), right?

We have API to create/delete template. For example: https://github.com/apo-ger/katib/blob/ae941c4cc66747a7e0db363ec7c9e1f877a1b9c9/pkg/new-ui/v1beta1/backend.go#L280.
The old UI has feature to CRUD Trial templates, but the new UI still don't support it, tracking here: #1440. cc @kimwnasptd
We might need to have discussion around this. Should we show Trial templates in kubeflow namespace or just avoid it.

We might need to have discussion around this. Should we show Trial templates in kubeflow namespace or just avoid it.

+1 on having a discussion around this. Although I think the current approach makes sense:

An admin adds TrialTemplate (ConfigMap) in the kubeflow namespace. These are visible from all users

But the backend won't allow users to edit/delete ConfigMaps in the kubeflow namespace. For these actions there are SubjectAccessReview checks in place.

If an admin wants to allow a user to manipulate the global (kubeflow namespace) ConfigMaps then they just need to give them RBAC permissions

Users will then see/interact-with ConfigMaps in other namespaces that they have permissions for

pkg/new-ui/v1beta1/util.go

* Add constants for CRUD actions * Add plural for experiments and suggestions as constants * Add GetUsername logic under IsAuthorized and handle errors properly * Have APP_DISABLE_AUTH by default as true, since currently Katib doesn't support this feature in standalone mode. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

andreyvelich · 2022-11-22T15:02:57Z

Thanks a lot for implementing this @apo-ger!
/lgtm
/assign @kimwnasptd @johnugeorge @tenzen-y

apo-ger · 2022-11-22T16:49:10Z

@andreyvelich @johnugeorge @tenzen-y @kimwnasptd Thank you for the review!

kimwnasptd · 2022-11-23T16:03:24Z

The current code looks good from my side. @andreyvelich thank you for the thorough review!

/lgtm

tenzen-y

@apo-ger Thanks a lot!

/lgtm

kimwnasptd · 2022-11-24T08:46:19Z

🎉 🎉

/approve

andreyvelich · 2022-11-25T23:36:20Z

/approve
@apo-ger Feel free to unhold the PR when you are ready.

google-oss-prow · 2022-11-25T23:36:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, apo-ger, kimwnasptd

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:

~~OWNERS~~ [andreyvelich]

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

apo-ger · 2022-11-28T10:28:08Z

🎉
/unhold

google-oss-prow bot requested review from anencore94 and tenzen-y October 25, 2022 11:47

google-oss-prow bot added the size/XL label Oct 25, 2022

apo-ger force-pushed the feature-arrikto-apoger-katib-authnz-sar branch from dde5a0f to 0b6e3c1 Compare October 25, 2022 15:04

google-oss-prow bot assigned kimwnasptd Oct 25, 2022

google-oss-prow bot added the do-not-merge/hold label Oct 25, 2022

johnugeorge mentioned this pull request Nov 4, 2022

Katib experiment detail page 500 error after refresh #1967

Closed

kimwnasptd mentioned this pull request Nov 8, 2022

Kubeflow 1.9 Enterprise grade Agenda kubeflow/kubeflow#6662

Closed

apo-ger commented Nov 17, 2022

View reviewed changes

tenzen-y reviewed Nov 17, 2022

View reviewed changes

apo-ger force-pushed the feature-arrikto-apoger-katib-authnz-sar branch from aa45849 to a343ef1 Compare November 17, 2022 11:57

apo-ger force-pushed the feature-arrikto-apoger-katib-authnz-sar branch from a343ef1 to 0377ed3 Compare November 17, 2022 12:46

andreyvelich reviewed Nov 17, 2022

View reviewed changes

apo-ger added 4 commits November 18, 2022 10:30

UI(front): Provide a namespace as a query parameter

e81457d

This is needed for the new /katib/fetch_namespaced_experiments route. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

Update README for running locally without auth

d4a486e

Update the README of the web app to expose that devs should set APP_DISABLE_AUTH=true when running locally, since there's no authnz when running locally. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

remove duplicated variable types

7742798

Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

Review fixes

86f190a

* proper error handling. * switch to Go's build-in errors package. * set appropriate verbs when constructing SAR objects. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

apo-ger force-pushed the feature-arrikto-apoger-katib-authnz-sar branch 2 times, most recently from 4e83b28 to e436760 Compare November 18, 2022 08:46

andreyvelich mentioned this pull request Nov 18, 2022

[SDK] Create API to get Trial metrics from Katib DB #2022

Closed

andreyvelich reviewed Nov 18, 2022

View reviewed changes

apo-ger force-pushed the feature-arrikto-apoger-katib-authnz-sar branch from 7a581e3 to ae941c4 Compare November 21, 2022 11:10

andreyvelich reviewed Nov 21, 2022

View reviewed changes

pkg/new-ui/v1beta1/util.go Outdated Show resolved Hide resolved

apo-ger force-pushed the feature-arrikto-apoger-katib-authnz-sar branch from ae941c4 to ad1e3b0 Compare November 22, 2022 08:12

google-oss-prow bot assigned johnugeorge, tenzen-y and andreyvelich Nov 22, 2022

google-oss-prow bot added the lgtm label Nov 22, 2022

tenzen-y reviewed Nov 23, 2022

View reviewed changes

google-oss-prow bot added the approved label Nov 25, 2022

google-oss-prow bot removed the do-not-merge/hold label Nov 28, 2022

google-oss-prow bot merged commit 831e1d3 into kubeflow:master Nov 28, 2022

apo-ger deleted the feature-arrikto-apoger-katib-authnz-sar branch November 28, 2022 11:05

apo-ger mentioned this pull request Nov 29, 2022

Update manifests to enable authorization check mechanisms for katib-UI in kubeflow mode #2041

Merged

kimwnasptd mentioned this pull request Mar 17, 2023

[0.15-rc.1] 401 error: user header not present #2126

Closed

juliusvonkohout mentioned this pull request May 24, 2024

Kubeflow 1.10 Enterprise grade Agenda kubeflow/manifests#2727

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authorization mechanisms in new Katib UI backend #1983

Add authorization mechanisms in new Katib UI backend #1983

apo-ger commented Oct 25, 2022 •

edited

Loading

johnugeorge commented Oct 25, 2022

andreyvelich commented Oct 25, 2022

andreyvelich commented Oct 25, 2022

coveralls commented Oct 26, 2022 •

edited

Loading

apo-ger commented Oct 31, 2022

apo-ger commented Nov 16, 2022 •

edited

Loading

johnugeorge commented Nov 16, 2022

apo-ger Nov 17, 2022 •

edited

Loading

tenzen-y left a comment

apo-ger commented Nov 17, 2022 •

edited

Loading

andreyvelich commented Nov 17, 2022

andreyvelich left a comment

apo-ger commented Nov 18, 2022 •

edited

Loading

apo-ger commented Nov 18, 2022 •

edited

Loading

andreyvelich commented Nov 18, 2022 •

edited

Loading

andreyvelich Nov 18, 2022

apo-ger Nov 21, 2022

johnugeorge commented Nov 18, 2022

andreyvelich Nov 21, 2022

apo-ger Nov 21, 2022 •

edited

Loading

apo-ger Nov 21, 2022

andreyvelich Nov 21, 2022

kimwnasptd Nov 23, 2022

andreyvelich commented Nov 22, 2022

apo-ger commented Nov 22, 2022

kimwnasptd commented Nov 23, 2022

tenzen-y left a comment

kimwnasptd commented Nov 24, 2022

andreyvelich commented Nov 25, 2022

google-oss-prow bot commented Nov 25, 2022

apo-ger commented Nov 28, 2022

Add authorization mechanisms in new Katib UI backend #1983

Add authorization mechanisms in new Katib UI backend #1983

Conversation

apo-ger commented Oct 25, 2022 • edited Loading

johnugeorge commented Oct 25, 2022

andreyvelich commented Oct 25, 2022

andreyvelich commented Oct 25, 2022

coveralls commented Oct 26, 2022 • edited Loading

apo-ger commented Oct 31, 2022

apo-ger commented Nov 16, 2022 • edited Loading

johnugeorge commented Nov 16, 2022

apo-ger Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

apo-ger commented Nov 17, 2022 • edited Loading

andreyvelich commented Nov 17, 2022

andreyvelich left a comment

Choose a reason for hiding this comment

apo-ger commented Nov 18, 2022 • edited Loading

apo-ger commented Nov 18, 2022 • edited Loading

andreyvelich commented Nov 18, 2022 • edited Loading

andreyvelich Nov 18, 2022

Choose a reason for hiding this comment

apo-ger Nov 21, 2022

Choose a reason for hiding this comment

johnugeorge commented Nov 18, 2022

andreyvelich Nov 21, 2022

Choose a reason for hiding this comment

apo-ger Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

apo-ger Nov 21, 2022

Choose a reason for hiding this comment

andreyvelich Nov 21, 2022

Choose a reason for hiding this comment

kimwnasptd Nov 23, 2022

Choose a reason for hiding this comment

andreyvelich commented Nov 22, 2022

apo-ger commented Nov 22, 2022

kimwnasptd commented Nov 23, 2022

tenzen-y left a comment

Choose a reason for hiding this comment

kimwnasptd commented Nov 24, 2022

andreyvelich commented Nov 25, 2022

google-oss-prow bot commented Nov 25, 2022

apo-ger commented Nov 28, 2022

apo-ger commented Oct 25, 2022 •

edited

Loading

coveralls commented Oct 26, 2022 •

edited

Loading

apo-ger commented Nov 16, 2022 •

edited

Loading

apo-ger Nov 17, 2022 •

edited

Loading

apo-ger commented Nov 17, 2022 •

edited

Loading

apo-ger commented Nov 18, 2022 •

edited

Loading

apo-ger commented Nov 18, 2022 •

edited

Loading

andreyvelich commented Nov 18, 2022 •

edited

Loading

apo-ger Nov 21, 2022 •

edited

Loading