From df961073d30fdc61edd46883a80f1f5b1e6178d5 Mon Sep 17 00:00:00 2001 From: Sven Nobis Date: Thu, 4 Sep 2025 21:50:40 +0200 Subject: [PATCH] feat: Add support to run notebooks on its own subdomains in Istio This new feature makes it possible to host notebooks on their own subdomains when Istio is used. 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. Signed-off-by: Lorin Lehawany Signed-off-by: Sven Nobis --- components/notebook-controller/README.md | 12 +- .../controllers/notebook_controller.go | 204 ++++++++++++++---- 2 files changed, 177 insertions(+), 39 deletions(-) diff --git a/components/notebook-controller/README.md b/components/notebook-controller/README.md index c8c9db8f0..6b0befe39 100644 --- a/components/notebook-controller/README.md +++ b/components/notebook-controller/README.md @@ -34,7 +34,17 @@ All other fields will be filled in with default value if not specified. | --- | --- | |ADD_FSGROUP| If the value is true or unset, fsGroup: 100 will be included in the pod's security context. If this value is present and set to false, it will suppress the automatic addition of fsGroup: 100 to the security context of the pod.| |DEV| If the value is false or unset, then the default implementation of the Notebook Controller will be used. If the admins want to use a custom implementation from their local machine, they should set this value to true.| - +|USE_ISTIO| If the value is set to true, Istio will be used to route traffic. If set to false, Istio routing will be disabled.| +|ISTIO_GATEWAY|Name of the Istio Gateway resource used to route traffic to notebook servers. Only used if ``USE_ISTIO`` is enabled.| +|ISTIO_HOST|Specifies the host to use in the Istio VirtualService. Default is ``*``. Only used if ``USE_ISTIO`` is enabled.| +|ISTIO_USE_NOTEBOOK_SUBDOMAINS| If the value is true, notebooks hosted on a subdomain. This is recommended for security reasons but has some technical dependencies. If the value is set to false, secure subdomain feature is not enabled. Disabled by default.| +|ISTIO_HOST_NOTEBOOK| Domain template used by Istio to host notebooks (e.g., `${NAMESPACE}-notebook.kubeflow.example.com`). Required if ``ISTIO_USE_NOTEBOOK_SUBDOMAINS`` is enabled. | +|ISTIO_HOST_AUTH| Host used by Istio for handling authentication callbacks or login flows (e.g., `kubeflow.example.com`). Required if ``ISTIO_USE_NOTEBOOK_SUBDOMAINS`` is enabled. | +|ISTIO_AUTH_PATH|Specifies the path used for authentication callbacks or redirection when integrating with an identity provider through Istio. Default is ``/oauth2/``. Only used if ``ISTIO_USE_NOTEBOOK_SUBDOMAINS`` is enabled. | +|ENABLE_CULLING|If the value is true, the controller will do culling on idle notebooks. Not enabled by default.| +|CULL_IDLE_TIME|Specifies minutes until an idle notebook will be stopped. Default is 1 day.| +|IDLENESS_CHECK_PERIOD|Specifies minutes for the idleness check period. Default is 1 minute.| +|CLUSTER_DOMAIN| Specifies the Kubernetes internal cluster domain. Default is ``cluster.local``| ## Commandline parameters diff --git a/components/notebook-controller/controllers/notebook_controller.go b/components/notebook-controller/controllers/notebook_controller.go index a44075482..f6ba75e27 100644 --- a/components/notebook-controller/controllers/notebook_controller.go +++ b/components/notebook-controller/controllers/notebook_controller.go @@ -18,6 +18,7 @@ package controllers import ( "context" "encoding/json" + "errors" "fmt" "os" "reflect" @@ -468,7 +469,108 @@ func virtualServiceName(kfName string, namespace string) string { return fmt.Sprintf("notebook-%s-%s", namespace, kfName) } -func generateVirtualService(instance *v1beta1.Notebook) (*unstructured.Unstructured, error) { +func virtualServiceNameWithSuffix(kfName string, namespace string, suffix string) string { + return fmt.Sprintf("notebook-%s-%s-%s", namespace, kfName, suffix) +} + +func generateVirtualServiceForNotebookRedirect(name string, namespace string, prefix string, istioHostNotebook string, istioGateway string) (*unstructured.Unstructured, error) { + vsvc := &unstructured.Unstructured{} + vsvc.SetAPIVersion("networking.istio.io/v1alpha3") + vsvc.SetKind("VirtualService") + vsvc.SetName(virtualServiceNameWithSuffix(name, namespace, "notebook-redirect")) + vsvc.SetNamespace(namespace) + + istioHost := os.Getenv("ISTIO_HOST") + if len(istioHost) == 0 { + istioHost = "*" + } + if err := unstructured.SetNestedStringSlice(vsvc.Object, []string{istioHost}, "spec", "hosts"); err != nil { + return nil, fmt.Errorf("Set .spec.hosts error: %v", err) + + } + + if err := unstructured.SetNestedStringSlice(vsvc.Object, []string{istioGateway}, + "spec", "gateways"); err != nil { + return nil, fmt.Errorf("set .spec.gateways error: %v", err) + } + + // the http section of the istio VirtualService spec + http := []interface{}{ + map[string]interface{}{ + "match": []interface{}{ + map[string]interface{}{ + "uri": map[string]interface{}{ + "prefix": prefix, + }, + }, + }, + "redirect": map[string]interface{}{ + "authority": istioHostNotebook, + "redirectCode": int64(307), + }, + }, + } + + // add http section to istio VirtualService spec + if err := unstructured.SetNestedSlice(vsvc.Object, http, "spec", "http"); err != nil { + return nil, fmt.Errorf("set .spec.http error: %v", err) + } + + return vsvc, nil +} + +func generateVirtualServiceForAuthRedirect(name string, namespace string, prefix string, istioHostNotebook string, istioGateway string) (*unstructured.Unstructured, error) { + vsvc := &unstructured.Unstructured{} + vsvc.SetAPIVersion("networking.istio.io/v1alpha3") + vsvc.SetKind("VirtualService") + vsvc.SetName(virtualServiceNameWithSuffix(name, namespace, "auth-redirect")) + vsvc.SetNamespace(namespace) + + if err := unstructured.SetNestedStringSlice(vsvc.Object, []string{istioHostNotebook}, "spec", "hosts"); err != nil { + return nil, fmt.Errorf("set .spec.hosts error: %v", err) + } + + if err := unstructured.SetNestedStringSlice(vsvc.Object, []string{istioGateway}, + "spec", "gateways"); err != nil { + return nil, fmt.Errorf("set .spec.gateways error: %v", err) + } + + istioHostAuth := os.Getenv("ISTIO_HOST_AUTH") + if len(istioHostAuth) == 0 { + return nil, fmt.Errorf("invalid ISTIO_HOST_AUTH: When ISTIO_USE_NOTEBOOK_SUBDOMAINS is set, the ISTIO_HOST_AUTH environment variable must be defined.") + } + + istioAuthPath := os.Getenv("ISTIO_AUTH_PATH") + if len(istioAuthPath) == 0 { + istioAuthPath = "/oauth2/" + } + + // the http section of the istio VirtualService spec + http := []interface{}{ + map[string]interface{}{ + "match": []interface{}{ + map[string]interface{}{ + "uri": map[string]interface{}{ + "prefix": istioAuthPath, + }, + }, + }, + "redirect": map[string]interface{}{ + "authority": istioHostAuth, + "redirectCode": int64(307), + }, + }, + } + + // add http section to istio VirtualService spec + if err := unstructured.SetNestedSlice(vsvc.Object, http, "spec", "http"); err != nil { + return nil, fmt.Errorf("set .spec.http error: %v", err) + } + + return vsvc, nil +} + +func generateVirtualServices(instance *v1beta1.Notebook) ([]*unstructured.Unstructured, error) { name := instance.Name namespace := instance.Namespace clusterDomain := "cluster.local" @@ -492,20 +594,13 @@ func generateVirtualService(instance *v1beta1.Notebook) (*unstructured.Unstructu service := fmt.Sprintf("%s.%s.svc.%s", name, namespace, clusterDomain) vsvc := &unstructured.Unstructured{} + vsvcs := []*unstructured.Unstructured{vsvc} + vsvc.SetAPIVersion("networking.istio.io/v1alpha3") vsvc.SetKind("VirtualService") vsvc.SetName(virtualServiceName(name, namespace)) vsvc.SetNamespace(namespace) - istioHost := os.Getenv("ISTIO_HOST") - if len(istioHost) == 0 { - istioHost = "*" - } - if err := unstructured.SetNestedStringSlice(vsvc.Object, []string{istioHost}, "spec", "hosts"); err != nil { - return nil, fmt.Errorf("Set .spec.hosts error: %v", err) - - } - istioGateway := os.Getenv("ISTIO_GATEWAY") if len(istioGateway) == 0 { istioGateway = "kubeflow/kubeflow-gateway" @@ -515,6 +610,33 @@ func generateVirtualService(instance *v1beta1.Notebook) (*unstructured.Unstructu return nil, fmt.Errorf("set .spec.gateways error: %v", err) } + istioHost := os.Getenv("ISTIO_HOST") + if os.Getenv("ISTIO_USE_NOTEBOOK_SUBDOMAINS") == "true" { + istioHost = os.Getenv("ISTIO_HOST_NOTEBOOK") + if !strings.Contains(istioHost, "${NAMESPACE}") { + return nil, errors.New("invalid ISTIO_HOST_NOTEBOOK: When ISTIO_USE_NOTEBOOK_SUBDOMAINS is set, the placeholder ${NAMESPACE} must be defined in ISTIO_HOST_NOTEBOOK.") + } + istioHost = strings.ReplaceAll(istioHost, "${NAMESPACE}", namespace) + + vsvcForNotebookRedirect, err := generateVirtualServiceForNotebookRedirect(name, namespace, prefix, istioHost, istioGateway) + if err != nil { + return nil, fmt.Errorf("generate virtual service for notebook redirect error: %v", err) + } + + vsvcForAuthRedirect, err := generateVirtualServiceForAuthRedirect(name, namespace, prefix, istioHost, istioGateway) + if err != nil { + return nil, fmt.Errorf("generate virtual service for auth redirect error: %v", err) + } + + vsvcs = append(vsvcs, vsvcForAuthRedirect, vsvcForNotebookRedirect) + } else if len(istioHost) == 0 { + istioHost = "*" + } + if err := unstructured.SetNestedStringSlice(vsvc.Object, []string{istioHost}, "spec", "hosts"); err != nil { + return nil, fmt.Errorf("Set .spec.hosts error: %v", err) + + } + headersRequestSet := make(map[string]string) // If AnnotationHeadersRequestSet is present, use its values in "headers.request.set" if _, ok := annotations[AnnotationHeadersRequestSet]; ok && len(annotations[AnnotationHeadersRequestSet]) > 0 { @@ -535,7 +657,8 @@ func generateVirtualService(instance *v1beta1.Notebook) (*unstructured.Unstructu map[string]interface{}{ "headers": map[string]interface{}{ "request": map[string]interface{}{ - "set": headersRequestSetInterface, + "set": headersRequestSetInterface, + "remove": []interface{}{"Cookie"}, }, }, "match": []interface{}{ @@ -566,46 +689,51 @@ func generateVirtualService(instance *v1beta1.Notebook) (*unstructured.Unstructu return nil, fmt.Errorf("set .spec.http error: %v", err) } - return vsvc, nil + return vsvcs, nil } func (r *NotebookReconciler) reconcileVirtualService(instance *v1beta1.Notebook) error { log := r.Log.WithValues("notebook", instance.Namespace) - virtualService, err := generateVirtualService(instance) + virtualServices, err := generateVirtualServices(instance) if err != nil { log.Info("Unable to generate VirtualService...", err) return err } - if err := ctrl.SetControllerReference(instance, virtualService, r.Scheme); err != nil { - return err - } - // Check if the virtual service already exists. - foundVirtual := &unstructured.Unstructured{} - justCreated := false - foundVirtual.SetAPIVersion("networking.istio.io/v1alpha3") - foundVirtual.SetKind("VirtualService") - err = r.Get(context.TODO(), types.NamespacedName{Name: virtualServiceName(instance.Name, - instance.Namespace), Namespace: instance.Namespace}, foundVirtual) - if err != nil && apierrs.IsNotFound(err) { - log.Info("Creating virtual service", "namespace", instance.Namespace, "name", - virtualServiceName(instance.Name, instance.Namespace)) - err = r.Create(context.TODO(), virtualService) - justCreated = true - if err != nil { + + for _, virtualService := range virtualServices { + + if err := ctrl.SetControllerReference(instance, virtualService, r.Scheme); err != nil { return err } - } else if err != nil { - return err - } - - if !justCreated && reconcilehelper.CopyVirtualService(virtualService, foundVirtual) { - log.Info("Updating virtual service", "namespace", instance.Namespace, "name", - virtualServiceName(instance.Name, instance.Namespace)) - err = r.Update(context.TODO(), foundVirtual) - if err != nil { + // Check if the virtual service already exists. + foundVirtual := &unstructured.Unstructured{} + justCreated := false + foundVirtual.SetAPIVersion("networking.istio.io/v1alpha3") + foundVirtual.SetKind("VirtualService") + virtualServiceName := virtualService.GetName() + virtualServiceNamespace := virtualService.GetNamespace() + err = r.Get(context.TODO(), types.NamespacedName{Name: virtualServiceName, Namespace: virtualServiceNamespace}, foundVirtual) + if err != nil && apierrs.IsNotFound(err) { + log.Info("Creating virtual service", "namespace", virtualServiceNamespace, "name", + virtualServiceName) + err = r.Create(context.TODO(), virtualService) + justCreated = true + if err != nil { + return err + } + } else if err != nil { return err } + + if !justCreated && reconcilehelper.CopyVirtualService(virtualService, foundVirtual) { + log.Info("Updating virtual service", "namespace", virtualServiceNamespace, "name", + virtualServiceName) + err = r.Update(context.TODO(), foundVirtual) + if err != nil { + return err + } + } } return nil