feat: Add support to run notebooks on its own subdomains in Istio

[sven-ernw]

This PR makes it possible to host notebooks on their own subdomains when Istio is used by adding this feature to the Kubeflow notebook-controller. This isolates the notebook's origin from the dashboard / Kubeflow API origin in the browser and addresses a security problem that allows session hijacking through a malicious notebook.

This pull request addresses the security issue described in GHSA-qjw6-hpc7-w36h

The relevant changes to the community-distribution can be found in pull request kubeflow/community-distribution#3550 .

Please note, we will add the feature to v2 once the feature is approved.

(reopend PR from !7742 in kubeflow/kubeflow)

Motivation / Why this change is needed

This change addresses a security problem that allows session hijacking through a malicious notebook:

An attacker can log session cookies by misusing the Notebook feature: An attacker can create a Notebook with a custom image that logs the cookies / trigger API requests. Afterwards, they must convince the victim to visit the URL of the Notebook (a phishing attack). This is a classic phishing attack scenario.

Disallowing custom images wouldn't be sufficient because you can achieve the same thing with a few extra steps using one of default images.

The problem relies on the fact that the Notebooks and the Kubeflow Dashboard / APIs are in the Browser's same origin. Thus, any attacker-controlled site would be able to access the Kubeflow API in the context of the victim.

To prevent this attack, the authenticating cookies need to be removed from requests forwarded to Notebook's Pods, and the notebooks need to be hosted on a different domain. This PR accomplishes this.

A proof of concept that demonstrates the problem is available but will not be disclosed until the problem is fixed.

Request for Feedback

At the moment, we are removing all cookie headers for security reasons before the request is forwarded to the notebook container. This breaks the functionality in some notebook images. So the goal is to remove only the authenticating cookie and keep the remaining cookies..

Our idea is to use a Istio Wasm Plugin that accomplishes this.

As discussed with @thesuperzapper, we will ad a envoy filter to remove the authorizing cookie. This will be part of a pull request in kubeflow-manifests.

What this PR accomplishes

1. Set individual hosts based on the environment variable ISTIO_HOST_NOTEBOOK in the notebook's VirtualServices when ISTIO_USE_NOTEBOOK_SUBDOMAINS is set to prevent access to the dashboard / APIs from attacker-controlled notebooks through the browser.
2. Implementing the authentication flow on the subdomains by redirecting unauthenticated requests to the host configured in ISTIO_HOST_AUTH for the ISTIO_AUTH_PATH.
3. Remove authenticating token from the request before it is forwarded to the notebook container to prevent session token stealing (see line 664 in components/notebook-controller/controllers/notebook_controller.go‎). The cookie header cannot be removed as cookies are required by some notebook images. We remove the authenticating cookie through an EnvoyFilter. This is part of security: Prevent session hijacking through a malicious notebook.  community-distribution#3550 and not this PR.

This pull request does not change any default behavior of the notebook's controller. Unless ISTIO_USE_NOTEBOOK_SUBDOMAINS is explicitly set, the notebook controller works as before.

Open Tasks (done)

This PR is a draft at the moment because the following parts are work in progress:

1. At the moment, we are removing all cookie headers for security reasons before the request is forwarded to the notebook container. This breaks the functionality in some notebook images. So the goal is to remove only the authenticating cookie and keep the remaining cookies. See Request for Feedback
   • We fixed this problem with an EnvoyFilter. This will not be part of this pull request as it will be part of the Kubeflow manifests repository. Thanks to @thesuperzapper for the productive discussion last year.
2. The subdomain feature requires additional configuration in the Kubeflow installation to make it fully functional (see configuration below). This will be documented and provided in a separate pull request in the manifests repository, see security: Prevent session hijacking through a malicious notebook.  community-distribution#3550
3. Unit test code for the new feature

Configuration

The subdomain feature is disabled by default as the setup has some prerequisites and is a breaking change. It requires a wildcard domain and wildcard TLS certificate or automated domain and certificate management.

To enable the feature, do the following configuration steps:

1. Configure the following environment variables in the notebook-controller:

ISTIO_USE_NOTEBOOK_SUBDOMAINS: true
ISTIO_HOST_NOTEBOOK: "${NAMESPACE}-notebook.kubeflow.example.com"
ISTIO_HOST_AUTH: "kubeflow.example.com"
ISTIO_HOST_AUTH_PATH: "/oauth2/"

2. The configmap of oauth2 must also be adjusted by enabling cookies for subdomains. Edit the oauth2_proxy.cfg configuration of the oauth2-proxy-hk55gm96k4 ConfigMap in the oauth2-proxy namespace and add the following setting:

cookie_domains = "kubeflow.example.com"

Detailed instructions are part of the pull request !3550 in kubeflow-manifests.

Notes

Setting forwardOriginalToken to false is not a solution as this would break existing functionality. We rather remove the token from requests to the notebooks only.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign thesuperzapper for approval. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• components/notebook-controller/OWNERS

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

[sven-ernw]

Hi @kimwnasptd @juliusvonkohout @thesuperzapper @ederign,
we can join the Notebooks meeting next Thursday to discuss the reason for this feature and request for feedback part if you like.

CC: @lorinl

[andyatmiami]

/area v1

[juliusvonkohout]

I am still struggling to find time to review this. Maybe after GSOC has finished in October.

[sven-ernw]

Hi Julius,
No worries, I'm currently implementing the patch for the open tasks (see above) and will notice you once everything is finished.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed if no further activity occurs.
Thank you for your contributions.

Members may comment /lifecycle frozen to prevent this pull request from being marked as stale.

[sven-ernw]

I will finish the open tasks of the MR next month.

[juliusvonkohout]

I will finish the open tasks of the MR next month.

It could make sense to align first in the WG calls or on slack with @andyatmiami @ederign @thesuperzapper @kimwnasptd

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed if no further activity occurs.
Thank you for your contributions.

Members may comment /lifecycle frozen to prevent this pull request from being marked as stale.

[sven-ernw]

The patch is still work in progress.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed if no further activity occurs.
Thank you for your contributions.

Members may comment /lifecycle frozen to prevent this pull request from being marked as stale.

[juliusvonkohout]

Please always use rebase, not merge from master. Rebase kaaeps a valid commit history.

[sven-ernw]

Thanks @juliusvonkohout . Did a rebase.

The feature is now complete.

[juliusvonkohout]

@thesuperzapper @christian-heusel for review

[Copilot]

Pull request overview

Adds opt-in Istio subdomain routing to isolate notebooks from Kubeflow dashboard/API origins and support the external security hardening flow.

Changes:

• Generates notebook, authentication, and legacy-route redirect VirtualServices.
• Adds controller configuration and documentation.
• Adds fixture-based VirtualService tests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
components/notebook-controller/README.md	Documents Istio settings.
controllers/test_fixtures/notebook_controller_virtualservice_test_subdomains.yaml	Covers default subdomain routing.
controllers/test_fixtures/notebook_controller_virtualservice_test_subdomains_different_auth_path.yaml	Covers custom authentication paths.
controllers/test_fixtures/notebook_controller_virtualservice_test_rewrite.yaml	Covers URI rewriting.
controllers/test_fixtures/notebook_controller_virtualservice_test_istio_host_set.yaml	Covers custom Istio hosts.
controllers/test_fixtures/notebook_controller_virtualservice_test_istio_gateway_set.yaml	Covers custom gateways.
controllers/test_fixtures/notebook_controller_virtualservice_test_headers.yaml	Covers request headers.
controllers/test_fixtures/notebook_controller_virtualservice_test_default_config.yaml	Covers default routing.
controllers/test_fixtures/notebook_controller_virtualservice_test_cluster_domain_set.yaml	Covers custom cluster domains.
controllers/notebook_controller.go	Implements multi-VirtualService routing and reconciliation.
controllers/notebook_controller_test.go	Adds generation tests and fixture decoding.
config/manager/params.env	Adds subdomain configuration defaults.
config/manager/manager.yaml	Injects new controller environment variables.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sven-ernw]

@thesuperzapper @christian-heusel I have reviewed and fixed all suggestions from the CoPilot review.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

components/notebook-controller/controllers/notebook_controller_test.go:643

• This cleanup only restores variables that existed before the test. TestReconcileVirtualServiceDeletesStaleRedirects sets ISTIO_HOST_NOTEBOOK and ISTIO_HOST_AUTH even when they were originally absent, so those values leak into later package tests. Register cleanup for both original states and unset keys that were absent.

		oldValue, hadValue := os.LookupEnv(key)
		if hadValue {
			t.Cleanup(func() {
				os.Setenv(key, oldValue)
			})
		}

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

[sven-ernw]

This is true. Good catch because Istio does not documented this feature.

[sven-ernw]

We have added the environment setting ISTIO_DISABLE_ENVOY_HEADER_MANIPULATION to disable envoy's substitution commands syntax in headers. This should be enabled for security reasons because this prevents access token leakage to user-controlled notebooks.

It is not enabled by default to avoid breaking changes. What do you think @thesuperzapper @christian-heusel ?

See commit: ed4c42b for details.

[sven-ernw]

@thesuperzapper @christian-heusel
Addressed all feedback from CoPilot,