Update manifests to enable authorization check mechanisms for katib-U…

…I in kubeflow mode (#2041)

* Upgrade manifests to enable authorization check mechanisms for katib-UI

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]>

* UI(back): Secure /katib/fetch_trial/ route

Introduce authn/authz checks in the backend

Signed-off-by: Apostolos Gerakaris <[email protected]>

* review: Remove duplicate dependencies

Signed-off-by: Apostolos Gerakaris <[email protected]>

* review: Move patch to a separate file

Signed-off-by: Apostolos Gerakaris <[email protected]>

Signed-off-by: Apostolos Gerakaris <[email protected]>

manifests/v1beta1/installs/katib-with-kubeflow/istio-authorizationpolicy.yaml

@@ -0,0 +1,15 @@
+---
+apiVersion: security.istio.io/v1beta1
+kind: AuthorizationPolicy
+metadata:
+  name: katib-ui
+  namespace: kubeflow
+spec:
+  action: ALLOW
+  selector:
+    matchLabels:
+      katib.kubeflow.org/component: ui
+  rules:
+    - from:
+        - source:
+            principals: ["cluster.local/ns/istio-system/sa/istio-ingressgateway-service-account"]

manifests/v1beta1/installs/katib-with-kubeflow/kustomization.yaml

@@ -7,6 +7,7 @@ resources:
   # Kubeflow Katib components.
   - kubeflow-katib-roles.yaml
   - ui-virtual-service.yaml
+  - istio-authorizationpolicy.yaml
 images:
   - name: docker.io/kubeflowkatib/katib-controller
     newName: docker.io/kubeflowkatib/katib-controller
@@ -21,6 +22,27 @@ images:
 patchesStrategicMerge:
   - patches/remove-namespace.yaml
 
+patches:
+  # Extend RBAC permission list of katib-ui so it can
+  # create SubjectAccessReview resources.
+  - target:
+      kind: ClusterRole
+      name: katib-ui
+      group: rbac.authorization.k8s.io
+      version: v1
+    path: patches/ui-rbac.yaml
+  # Enable RBAC authz checks in UI's backend.
+  - target:
+      version: v1
+      kind: Deployment
+      name: katib-ui
+    path: patches/enable-ui-authz-checks.yaml
+  # Allow istio sidecar injection in katib-UI Pod.
+  - target:
+      kind: Deployment
+      name: katib-ui
+    path: patches/istio-sidecar-injection.yaml
+
 vars:
   - fieldref:
       fieldPath: metadata.namespace

manifests/v1beta1/installs/katib-with-kubeflow/patches/enable-ui-authz-checks.yaml

@@ -0,0 +1,6 @@
+---
+- op: add
+  path: /spec/template/spec/containers/0/env/-
+  value:
+    name: APP_DISABLE_AUTH
+    value: "false"

manifests/v1beta1/installs/katib-with-kubeflow/patches/istio-sidecar-injection.yaml

@@ -0,0 +1,10 @@
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: "katib-ui"
+spec:
+  template:
+    metadata:
+      annotations:
+        sidecar.istio.io/inject: "true"

manifests/v1beta1/installs/katib-with-kubeflow/patches/ui-rbac.yaml

@@ -0,0 +1,7 @@
+---
+- op: add
+  path: /rules/-
+  value:
+    apiGroups: [authorization.k8s.io]
+    resources: [subjectaccessreviews]
+    verbs: [create]

pkg/new-ui/v1beta1/backend.go

@@ -40,7 +40,6 @@ import (
 
 	common "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1"
 	mccommon "github.com/kubeflow/katib/pkg/metricscollector/v1beta1/common"
-	apiv1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
@@ -565,8 +564,35 @@ func (k *KatibUIHandler) FetchSuggestion(w http.ResponseWriter, r *http.Request)
 
 // FetchTrial gets trial in specific namespace.
 func (k *KatibUIHandler) FetchTrial(w http.ResponseWriter, r *http.Request) {
-	trialName := r.URL.Query()["trialName"][0]
-	namespace := r.URL.Query()["namespace"][0]
+	namespaces, ok := r.URL.Query()["namespace"]
+	if !ok {
+		log.Printf("No namespace provided in Query parameters! Provide a 'namespace' param")
+		err := errors.New("no 'namespace' provided")
+		http.Error(w, err.Error(), http.StatusBadRequest)
+		return
+	}
+
+	trialNames, ok := r.URL.Query()["trialName"]
+	if !ok {
+		log.Printf("No trialName provided in Query parameters! Provide a 'trialName' param")
+		err := errors.New("no 'trialName' provided")
+		http.Error(w, err.Error(), http.StatusBadRequest)
+		return
+	}
+
+	trialName := trialNames[0]
+	namespace := namespaces[0]
+
+	user, err := IsAuthorized(consts.ActionTypeGet, namespace, consts.PluralTrial, "", trialName, trialsv1beta1.SchemeGroupVersion, k.katibClient.GetClient(), r)
+	if user == "" && err != nil {
+		log.Printf("No user provided in kubeflow-userid header.")
+		http.Error(w, err.Error(), http.StatusUnauthorized)
+		return
+	} else if err != nil {
+		log.Printf("The user: %s is not authorized to get trial: %s from namespace: %s \n", user, trialName, namespace)
+		http.Error(w, err.Error(), http.StatusForbidden)
+		return
+	}
 
 	trial, err := k.katibClient.GetTrial(trialName, namespace)
 	if err != nil {
@@ -652,7 +678,7 @@ func (k *KatibUIHandler) FetchTrialLogs(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	podLogOpts := apiv1.PodLogOptions{}
+	podLogOpts := corev1.PodLogOptions{}
 	podLogOpts.Container = trial.Spec.PrimaryContainerName
 	if trial.Spec.MetricsCollector.Collector.Kind == common.StdOutCollector {
 		podLogOpts.Container = mccommon.MetricLoggerCollectorContainerName
@@ -715,7 +741,7 @@ An example can be found here: https://github.com/kubeflow/katib/blob/7bf39225f72
 }
 
 // fetchPodLogs returns logs of a pod for the given job name and namespace
-func fetchPodLogs(clientset *kubernetes.Clientset, namespace string, podName string, podLogOpts apiv1.PodLogOptions) (string, error) {
+func fetchPodLogs(clientset *kubernetes.Clientset, namespace string, podName string, podLogOpts corev1.PodLogOptions) (string, error) {
 	req := clientset.CoreV1().Pods(namespace).GetLogs(podName, &podLogOpts)
 	podLogs, err := req.Stream(context.Background())
 	if err != nil {