RHOAIENG-11155: Better explanation of 'Authorize Access' UI (#449)

* Add methods to handle OAuth client secret generation

- Add volumes to store the oauth-client configuration;
- Add extra parameters to container creation, including --client-id, --client-secret and --scope;
- Add method to generate the secret randomly when authenticating using OAuth;
- Add link between the notebook's route, user's generated oauth secret and OAuthClient config;

* Add RBAC role to have access to OAuthClient objects

* Fix pointer.Int32Ptr deprecation on oauth-client test

* Add OAuthClient role to odh-notebook-controller's kubebuilder config

* Change error message on OAuth Client creation error

* Add validation when getting route from OAuthClient reconciler

* Fix pointer.Int32Ptr deprecation in webhook

* Fix linter complaints about r.Client presence

* Remove oauthClient ownership code regarding rhods-operator

* Add proper description to empty comments

* Add label for notebook-owner to the oauthclient object

* Add ForceOwnership back to OAuthClient Patch

Co-authored-by: Harshad Reddy Nalla <[email protected]>

* Fix linter complaints about last commits

---------

Co-authored-by: Harshad Reddy Nalla <[email protected]>

Author: daniellutz

components/odh-notebook-controller/config/rbac/role.yaml

@@ -65,6 +65,17 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - oauth.openshift.io
+  resources:
+  - oauthclients
+  verbs:
+  - create
+  - get
+  - list
+  - patch
+  - update
+  - watch
 - apiGroups:
   - rbac.authorization.k8s.io
   resources:

components/odh-notebook-controller/controllers/notebook_controller.go

@@ -73,6 +73,7 @@ type OpenshiftNotebookReconciler struct {
 // +kubebuilder:rbac:groups="",resources=services;serviceaccounts;secrets;configmaps,verbs=get;list;watch;create;update;patch
 // +kubebuilder:rbac:groups=config.openshift.io,resources=proxies,verbs=get;list;watch
 // +kubebuilder:rbac:groups=networking.k8s.io,resources=networkpolicies,verbs=get;list;watch;create;update;patch
+// +kubebuilder:rbac:groups=oauth.openshift.io,resources=oauthclients,verbs=get;list;watch;create;update;patch
 // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=get;list;watch;create;update;patch
 // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups="image.openshift.io",resources=imagestreams,verbs=list;get
@@ -234,6 +235,12 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re
 			if err != nil {
 				return ctrl.Result{}, err
 			}
+
+			// Call the OAuthClient reconciler
+			err = r.ReconcileOAuthClient(notebook, ctx)
+			if err != nil {
+				return ctrl.Result{}, err
+			}
 		} else {
 			// Call the route reconciler (see notebook_route.go file)
 			err = r.ReconcileRoute(notebook, ctx)

components/odh-notebook-controller/controllers/notebook_controller_test.go

@@ -630,6 +630,15 @@ var _ = Describe("The Openshift Notebook controller", func() {
 									},
 								},
 							},
+							{
+								Name: "oauth-client",
+								VolumeSource: corev1.VolumeSource{
+									Secret: &corev1.SecretVolumeSource{
+										SecretName:  Name + "-oauth-client",
+										DefaultMode: ptr.To[int32](420),
+									},
+								},
+							},
 							{
 								Name: "tls-certificates",
 								VolumeSource: corev1.VolumeSource{
@@ -1000,6 +1009,9 @@ func createOAuthContainer(name, namespace string) corev1.Container {
 			"--upstream-ca=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt",
 			"--email-domain=*",
 			"--skip-provider-button",
+			`--client-id=` + name + `-` + namespace + `-oauth-client`,
+			"--client-secret-file=/etc/oauth/client/secret",
+			"--scope=user:info user:check-access",
 			`--openshift-sar={"verb":"get","resource":"notebooks","resourceAPIGroup":"kubeflow.org",` +
 				`"resourceName":"` + name + `","namespace":"$(NAMESPACE)"}`,
 			"--logout-url=https://example.notebook-url/notebook/" + namespace + "/" + name,
@@ -1048,6 +1060,10 @@ func createOAuthContainer(name, namespace string) corev1.Container {
 			},
 		},
 		VolumeMounts: []corev1.VolumeMount{
+			{
+				Name:      "oauth-client",
+				MountPath: "/etc/oauth/client",
+			},
 			{
 				Name:      "oauth-config",
 				MountPath: "/etc/oauth/config",

components/odh-notebook-controller/controllers/notebook_oauth.go

@@ -19,17 +19,23 @@ import (
 	"context"
 	"crypto/rand"
 	"encoding/base64"
+	"encoding/json"
+	"fmt"
+	"math/big"
 	"reflect"
 
 	"k8s.io/apimachinery/pkg/util/intstr"
 
 	nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1"
+	oauthv1 "github.com/openshift/api/oauth/v1"
 	routev1 "github.com/openshift/api/route/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
 const (
@@ -39,12 +45,24 @@ const (
 	// taken from https://catalog.redhat.com/software/containers/openshift4/ose-oauth-proxy/5cdb2133bed8bd5717d5ae64?image=66cefc14401df6ff4664ec43&architecture=amd64&container-tabs=overview
 	// and kept in sync with the manifests here and in ClusterServiceVersion metadata of opendatahub operator
 	OAuthProxyImage = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4f8d66597feeb32bb18699326029f9a71a5aca4a57679d636b876377c2e95695"
+
+	// Strings used in secret generation
+	letterRunes = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
+
+	// Complexity of generated secrets, this will be stored in a const for now, but in the future
+	// there is the possibility of creating a specific file to manage secrets, change the size of
+	// the complexity if required, etc. For now, a complexity of 16 will suffice.
+	SECRET_DEFAULT_COMPLEXITY = 16
 )
 
 type OAuthConfig struct {
 	ProxyImage string
 }
 
+type OAuthClientConfig struct {
+	Name string
+}
+
 // NewNotebookServiceAccount defines the desired service account object
 func NewNotebookServiceAccount(notebook *nbv1.Notebook) *corev1.ServiceAccount {
 	return &corev1.ServiceAccount{
@@ -209,39 +227,106 @@ func NewNotebookOAuthSecret(notebook *nbv1.Notebook) *corev1.Secret {
 	}
 }
 
+// The NewNotebookOAuthClientSecret will generate a random secret that will be used
+// by the OAuth proxy sidecar container to automatically accept the required permissions
+// for the user to access a notebook instead of showing up a page where the user needs to
+// aggree with content he might not fully understand
+// More info: https://issues.redhat.com/browse/RHOAIENG-11155
+func NewNotebookOAuthClientSecret(notebook *nbv1.Notebook) *corev1.Secret {
+	// Generate the client secret for the OAuth proxy
+	randomValue := make([]byte, SECRET_DEFAULT_COMPLEXITY)
+	for i := 0; i < SECRET_DEFAULT_COMPLEXITY; i++ {
+		num, err := rand.Int(rand.Reader, big.NewInt(int64(len(letterRunes))))
+		if err != nil {
+			fmt.Printf("Error generating secret: %v\n", err)
+		}
+		randomValue[i] = letterRunes[num.Int64()]
+	}
+
+	// Create a Kubernetes secret to store the cookie secret
+	return &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      notebook.Name + "-oauth-client",
+			Namespace: notebook.Namespace,
+			Labels: map[string]string{
+				"notebook-name": notebook.Name,
+			},
+		},
+		StringData: map[string]string{
+			"secret": string(randomValue),
+		},
+	}
+}
+
 // ReconcileOAuthSecret will manage the OAuth secret reconciliation required by
 // the notebook OAuth proxy
 func (r *OpenshiftNotebookReconciler) ReconcileOAuthSecret(notebook *nbv1.Notebook, ctx context.Context) error {
-	// Initialize logger format
-	log := r.Log.WithValues("notebook", notebook.Name, "namespace", notebook.Namespace)
-
 	// Generate the desired OAuth secret
-	desiredSecret := NewNotebookOAuthSecret(notebook)
+	desiredOAuthSecret := NewNotebookOAuthSecret(notebook)
+	_ = r.createSecret(notebook, ctx, desiredOAuthSecret)
+
+	// Generate the desired OAuthClientSecret
+	desiredOAuthClientSecret := NewNotebookOAuthClientSecret(notebook)
+	_ = r.createSecret(notebook, ctx, desiredOAuthClientSecret)
+
+	return nil
+}
+
+func (r *OpenshiftNotebookReconciler) ReconcileOAuthClient(notebook *nbv1.Notebook, ctx context.Context) error {
+	log := logf.FromContext(ctx)
 
-	// Create the OAuth secret if it does not already exist
-	foundSecret := &corev1.Secret{}
+	route := &routev1.Route{}
+	err := r.Get(ctx, types.NamespacedName{
+		Name:      notebook.Name,
+		Namespace: notebook.Namespace,
+	}, route)
+	if err != nil {
+		if apierrs.IsNotFound(err) {
+			log.Info("Route not found, cannot create OAuthClient yet", "route", notebook.Name)
+			return nil
+		}
+		log.Error(err, "Failed to get Route for OAuthClient")
+		return err
+	}
+
+	err = r.createOAuthClient(notebook, ctx)
+	if err != nil {
+		log.Error(err, "Unable to handle OAuthClient creation / update")
+		return err
+	}
+
+	return nil
+}
+
+func (r *OpenshiftNotebookReconciler) createSecret(notebook *nbv1.Notebook, ctx context.Context, desiredSecret *corev1.Secret) error {
+	log := logf.FromContext(ctx)
+
+	secret := &corev1.Secret{}
 	err := r.Get(ctx, types.NamespacedName{
 		Name:      desiredSecret.Name,
 		Namespace: notebook.Namespace,
-	}, foundSecret)
+	}, secret)
+
 	if err != nil {
 		if apierrs.IsNotFound(err) {
-			log.Info("Creating OAuth Secret")
-			// Add .metatada.ownerReferences to the OAuth secret to be deleted by
+			log.Info("Creating ", "secret", desiredSecret.Name)
+
+			// Add .metatada.ownerReferences to the OAuth client secret to be deleted by
 			// the Kubernetes garbage collector if the notebook is deleted
 			err = ctrl.SetControllerReference(notebook, desiredSecret, r.Scheme)
 			if err != nil {
-				log.Error(err, "Unable to add OwnerReference to the OAuth Secret")
+				log.Error(err, "Unable to add OwnerReference to secret ", desiredSecret.Name)
 				return err
 			}
-			// Create the OAuth secret in the Openshift cluster
+
+			// Create the OAuth client secret in the Openshift cluster
 			err = r.Create(ctx, desiredSecret)
 			if err != nil && !apierrs.IsAlreadyExists(err) {
-				log.Error(err, "Unable to create the OAuth Secret")
+				log.Error(err, "Unable to create secret ", desiredSecret.Name)
 				return err
 			}
 		} else {
-			log.Error(err, "Unable to fetch the OAuth Secret")
+			log.Error(err, "Unable to fetch secret")
 			return err
 		}
 	}
@@ -264,3 +349,64 @@ func (r *OpenshiftNotebookReconciler) ReconcileOAuthRoute(
 	notebook *nbv1.Notebook, ctx context.Context) error {
 	return r.reconcileRoute(notebook, ctx, NewNotebookOAuthRoute)
 }
+
+func (r *OpenshiftNotebookReconciler) createOAuthClient(notebook *nbv1.Notebook, ctx context.Context) error {
+	log := logf.FromContext(ctx)
+
+	// Get the route that will be used in the OAuthClient
+	oauthClientRoute := &routev1.Route{}
+	err := r.Get(ctx, types.NamespacedName{
+		Name:      notebook.Name,
+		Namespace: notebook.Namespace,
+	}, oauthClientRoute)
+	if err != nil {
+		log.Error(err, "Unable to retrieve route", "route", notebook.Name)
+		return err
+	}
+
+	// Get the secret that will be used in the OAuthClient
+	secret := &corev1.Secret{}
+	err = r.Get(ctx, types.NamespacedName{
+		Name:      notebook.Name + "-oauth-client",
+		Namespace: notebook.Namespace,
+	}, secret)
+	if err != nil {
+		log.Error(err, "Unable to retrieve secret", "secret", notebook.Name)
+		return err
+	}
+
+	stringData := string(secret.Data["secret"])
+
+	oauthClient := &oauthv1.OAuthClient{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "OAuthClient",
+			APIVersion: "oauth.openshift.io/v1",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: notebook.Name + "-" + notebook.Namespace + "-oauth-client",
+			Labels: map[string]string{
+				"notebook-owner": notebook.Name,
+			},
+		},
+		Secret:       stringData,
+		RedirectURIs: []string{"https://" + oauthClientRoute.Spec.Host},
+		GrantMethod:  oauthv1.GrantHandlerAuto,
+	}
+
+	err = r.Create(ctx, oauthClient)
+	if err != nil {
+		if apierrs.IsAlreadyExists(err) {
+			data, err := json.Marshal(oauthClient)
+			if err != nil {
+				return fmt.Errorf("failed to create OAuth Client: %w", err)
+			}
+			if err = r.Patch(ctx, oauthClient, client.RawPatch(types.ApplyPatchType, data),
+				client.ForceOwnership, client.FieldOwner("odh-notebook-controller")); err != nil {
+				return fmt.Errorf("failed to patch existing OAuthClient CR: %w", err)
+			}
+			return nil
+		}
+	}
+
+	return nil
+}

components/odh-notebook-controller/controllers/notebook_webhook.go

@@ -120,6 +120,9 @@ func InjectOAuthProxy(notebook *nbv1.Notebook, oauth OAuthConfig) error {
 			"--upstream-ca=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt",
 			"--email-domain=*",
 			"--skip-provider-button",
+			`--client-id=` + notebook.Name + `-` + notebook.Namespace + `-oauth-client`,
+			"--client-secret-file=/etc/oauth/client/secret",
+			"--scope=user:info user:check-access",
 			`--openshift-sar={"verb":"get","resource":"notebooks","resourceAPIGroup":"kubeflow.org",` +
 				`"resourceName":"` + notebook.Name + `","namespace":"$(NAMESPACE)"}`,
 		},
@@ -167,6 +170,10 @@ func InjectOAuthProxy(notebook *nbv1.Notebook, oauth OAuthConfig) error {
 			},
 		},
 		VolumeMounts: []corev1.VolumeMount{
+			{
+				Name:      "oauth-client",
+				MountPath: "/etc/oauth/client",
+			},
 			{
 				Name:      "oauth-config",
 				MountPath: "/etc/oauth/config",
@@ -222,6 +229,29 @@ func InjectOAuthProxy(notebook *nbv1.Notebook, oauth OAuthConfig) error {
 		*notebookVolumes = append(*notebookVolumes, oauthVolume)
 	}
 
+	// Add the OAuth Client configuration volume:
+	// https://pkg.go.dev/k8s.io/api/core/v1#Volume
+	clientVolumeExists := false
+	clientVolume := corev1.Volume{
+		Name: "oauth-client",
+		VolumeSource: corev1.VolumeSource{
+			Secret: &corev1.SecretVolumeSource{
+				SecretName:  notebook.Name + "-oauth-client",
+				DefaultMode: ptr.To[int32](420),
+			},
+		},
+	}
+	for index, volume := range *notebookVolumes {
+		if volume.Name == "oauth-client" {
+			(*notebookVolumes)[index] = clientVolume
+			clientVolumeExists = true
+			break
+		}
+	}
+	if !clientVolumeExists {
+		*notebookVolumes = append(*notebookVolumes, clientVolume)
+	}
+
 	// Add the TLS certificates volume:
 	// https://pkg.go.dev/k8s.io/api/core/v1#Volume
 	tlsVolumeExists := false

components/odh-notebook-controller/main.go

@@ -40,6 +40,7 @@ import (
 
 	nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1"
 	configv1 "github.com/openshift/api/config/v1"
+	oauthv1 "github.com/openshift/api/oauth/v1"
 	routev1 "github.com/openshift/api/route/v1"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
 	//+kubebuilder:scaffold:imports
@@ -56,6 +57,7 @@ func init() {
 	utilruntime.Must(nbv1.AddToScheme(scheme))
 	utilruntime.Must(routev1.AddToScheme(scheme))
 	utilruntime.Must(configv1.AddToScheme(scheme))
+	utilruntime.Must(oauthv1.AddToScheme(scheme))
 
 	//+kubebuilder:scaffold:scheme
 }