Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 24, 2023

Conversation

apo-ger
Copy link
Contributor

@apo-ger apo-ger commented Nov 29, 2022

What this PR does / why we need it:
This is a follow-up PR from: #1983

Currently, in kubeflow mode, any workload/pod can set the kubeflow-userid header and communicate with the backend of the Katib UI. Since the code for securing the backend is merged we need to provide a way to enable this feature.

Changes to install-with-kubeflow manifests:

  • Enable Istio sidecar injection for katib-ui component
  • Introduce AuthorizationPolicies:
    • katib-ui: Allow traffic only from the istio-ingressgateway towards the UI backend. We can trust IGW as we are sure it's setting the user header correctly.
  • Enable authz checks for katib-ui backend (set APP_DISABLE_AUTH ENV var to false)
  • Update ClusterRole to provide RBAC persmissions for the katib-ui so it can create SubjectAccessReview objects

Refs: #1547

@apo-ger
Copy link
Contributor Author

apo-ger commented Nov 29, 2022

@kimwnasptd @andreyvelich @johnugeorge In this PR I introduce the required manifest changes only for the katib-ui component. We could go one step further and enable istio for the rest of the components as well.

This would require additional AuthorizationPolicies to allow traffic but without the kubeflow-userid header set (to avoid impersonations) for certain components (e.g DB-manager, #2022).

Furthermore, in some cases we could apply narrower policies for hardening the security even more (e.g allow only katib-db-manager to talk to katib-mysql for storing/fetching metric logs).

As mentioned in #1983 (comment) and #1983 (comment) we need to test sidecar changes carefully.

Should we outline a full list of requirements for this effort?

@apo-ger apo-ger changed the title Upgrade manifests to enable authorization check mechanisms for katib-UI in kubeflow mode Update manifests to enable authorization check mechanisms for katib-UI in kubeflow mode Nov 29, 2022
@apo-ger apo-ger force-pushed the feature-authz-kf-katib branch 2 times, most recently from 699066e to 4590203 Compare December 13, 2022 17:57
@johnugeorge
Copy link
Member

@apo-ger We don't have a mechanism to test these changes. Did you test all workflows from Kubeflow dashboard?

@apo-ger
Copy link
Contributor Author

apo-ger commented Dec 19, 2022

Hi @johnugeorge, apologies for the late reply.

We don't have a mechanism to test these changes. Did you test all workflows from Kubeflow dashboard?

Enabling Istio sicecar injection for all Katib components would require us to introduce appropriate AuthorizationPolices. More specifically:

  • katib-controller: Allow traffic to the webhook port in order to let the K8s api server send traffic to this endpoint.
    • When an Experiment CR is submitted, the Katib Experiment mutating and validating webhook is called to set the default values for the Experiment CR and validate the CR.
  • katib-ui: Allow traffic only from the istio-ingressgateway towards the UI backend
  • katib-mysql: The db manager's persistence layer. Allow traffic only from db-manager.
  • katib-db-manager: Allow ALL traffic, but without the kubeflow-userid header set, as Katib jobs are currently not supporting Istio sidecar injection.

I have tested that the above AuthorizationPolicies are sufficient

@johnugeorge
Copy link
Member

Can you fix lint?

Also, which rule is responsible for katib-mysql? The db manager's persistence layer. Allow traffic only from db-manager.

@apo-ger apo-ger force-pushed the feature-authz-kf-katib branch 2 times, most recently from 0d793ad to 0e86177 Compare December 21, 2022 15:53
@andreyvelich
Copy link
Member

andreyvelich commented Dec 21, 2022

@apo-ger Please can we post-pone changes for other components (katib-controller, katib-db-manager, katib-mysql) to the next release ?
I think we should be very careful with enabling istio proxy for Katib Controller, etc. Since we don't have any E2Es to verify this change.

E.g. When Early Stopping is enabled, early stopping container queries data from the Katib DB: https://github.com/kubeflow/katib/blob/master/pkg/earlystopping/v1beta1/medianstop/service.py#L151.
What do you think @johnugeorge ?
/hold for the review

@johnugeorge
Copy link
Member

@apo-ger Can you update with Katib-UI changes in this PR and take other changes in a separate PR?

Changes to install-with-kubeflow manifests:

* Enable istio sidecar injection for katib-ui component

* Add AuthorizationPolicy to allow only istio-ingressgateway
  to talk to katib-ui [user traffic].

* Set APP_DISABLE_AUTH ENV var to false when in kubeflow-mode
  to enable authorization checks in UI's backend

* Extend the RBAC persmissions of katib-ui so it can crate SAR objects
  when in kubeflow-mode

Signed-off-by: Apostolos Gerakaris <[email protected]>
Introduce authn/authz checks in the backend

Signed-off-by: Apostolos Gerakaris <[email protected]>
@apo-ger
Copy link
Contributor Author

apo-ger commented Jan 23, 2023

I updated this PR to keep only the changes related to the katib-ui component as mentioned in #2041 (comment)

Signed-off-by: Apostolos Gerakaris <[email protected]>
@tenzen-y
Copy link
Member

@apo-ger Thanks for submitting this PR.
Can you update the PR description? The correct description helps to track changes.

Signed-off-by: Apostolos Gerakaris <[email protected]>
@tenzen-y
Copy link
Member

LGTM
/assign @johnugeorge @andreyvelich

@johnugeorge
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jan 23, 2023
@tenzen-y
Copy link
Member

@andreyvelich PTAL.

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thanks a lot for these updates @apo-ger!
/lgtm
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@tenzen-y
Copy link
Member

/hold cancel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants