feat(auth): Add OAuth client to service account #292

jan-law · 2021-11-05T19:01:41Z

Fixes #289

Lets the cryostat-sample service account access the OpenShift OAuth API by acting as an OAuth client. The redirect uri is created dynamically.

To request a scoped access token with the necessary permissions for the web-UI to create and manage recordings, I added permissions to the existing ClusterRole. The names of the ClusterRole and ServiceAccount are both referenced when making a backend request to the OAuth server in this PR: https://github.com/cryostatio/cryostat/pull/748

oc describe sa cryostat-sample
Name:                cryostat-sample
Namespace:           cryostat-operator-system
Labels:              app=cryostat
Annotations:        serviceaccounts.openshift.io/oauth-redirectreference.route:
                       {"metadata":{"creationTimestamp":null},"reference":{"group":"","kind":"Route","name":"cryostat-sample"}}
                     serviceaccounts.openshift.io/oauth-redirecturi.route: https://
Image pull secrets:  cryostat-sample-dockercfg-8x9n4
Mountable secrets:   cryostat-sample-dockercfg-8x9n4
                     cryostat-sample-token-vh7gw
Tokens:              cryostat-sample-token-glb24
                     cryostat-sample-token-vh7gw
Events:              <none>

More info:
https://docs.openshift.com/container-platform/4.9/authentication/using-service-accounts-in-applications.html

internal/controllers/common/resource_definitions/resource_definitions.go

ebaron · 2021-11-22T20:30:04Z

It worked for me doing the following:

$ make generate manifests manager 
$ BUILDAH_FORMAT=docker podman build -t quay.io/ebaron/cryostat-operator:latest .
$ podman push quay.io/ebaron/cryostat-operator:latest
$ make deploy IMG=quay.io/ebaron/cryostat-operator:latest
$ make create_cryostat_cr 
$ oc describe deploy cryostat-sample
[...]
    Environment:
[...]
      CRYOSTAT_OAUTH_CLIENT_ID:      cryostat-sample
      CRYOSTAT_OAUTH_ROLE:           cryostat-operator-cryostat

Normally, I'd use make oci-build instead of the first two steps, but this bypasses the test suite.

jan-law · 2021-12-01T20:18:54Z

This PR is ready for review. I'd also appreciate it if you could confirm the ClusterRole permissions give the exact permissions that the web-UI needs and nothing more.

ebaron · 2021-12-07T21:59:38Z

config/rbac/cryostat_role.yaml

-  - authorization.k8s.io
+  - ""
  resources:
-  - selfsubjectaccessreviews
+  - pods
  verbs:
  - create
+  - get
+- apiGroups:
+  - ""
+  resources:
+  - replicationcontrollers
+  - endpoints
+  verbs:
+  - get
+- apiGroups:
+  - operator.cryostat.io
+  resources:
+  - cryostats
+  verbs:
+  - create
+  - delete
+  - get
+- apiGroups:
+  - operator.cryostat.io
+  resources:
+  - flightrecorders
+  - recordings
+  verbs:
+  - create
+  - delete
+  - get
+  - patch
+- apiGroups:
+  - apps
+  resources:
+  - deployments
+  verbs:
+  - create
+  - get
+- apiGroups:
+  - apps
+  resources:
+  - daemonsets
+  - replicasets
+  - statefulsets
+  verbs:
+  - get


I think we'll need to move these permissions to a new ClusterRole after all. When I initially suggested reusing this ClusterRole, I thought the permission you'd need to add would be to use the OAuthClient API. Since the purpose of this is for a role-scoped token, we should add another ClusterRole which we don't bind to Cryostat's ServiceAccount with a ClusterRoleBinding. As it stands, this would give Cryostat the ability to perform these actions in any namespace.

Setting this PR to draft mode while I add the new ClusterRole

I've removed create TokenReviews and create SelfSubjectAccessReviews from the new ClusterRole. The backend can request a token with enough permissions using user:info user:check-access role:cryostat-operator-oauth-client:namespace.

Hey Janelle, I haven't tried it out yet, but do you know if the user:info scope allows the backend to create a TokenReview, or does that code need to be changed to use the OpenShift users API? I assumed it was the latter.

I'm assuming the user:info scope lets the backend create a TokenReview, because when I tested my latest image on OpenShift with the following ClusterRole permissions and scope=user%3Ainfo+user%3Acheck-access+role%3Acryostat-operator-oauth-client as the role scope, I didn't see any errors on the frontend or in the cryostat-sample logs.

Name: cryostat-operator-oauth-client Labels: <none> Annotations: <none> PolicyRule: Resources Non-Resource URLs Resource Names Verbs --------- ----------------- -------------- ----- flightrecorders.operator.cryostat.io [] [] [create delete get patch] recordings.operator.cryostat.io [] [] [create delete get patch] cryostats.operator.cryostat.io [] [] [create delete get] pods [] [] [create get] deployments.apps [] [] [create get] endpoints [] [] [get] replicationcontrollers [] [] [get] daemonsets.apps [] [] [get] replicasets.apps [] [] [get] statefulsets.apps [] [] [get]

I think what we're seeing is the TokenReview is being done using Cryostat's ServiceAccount: https://github.com/cryostatio/cryostat/blob/7c010b29b83677ba0a815315ddd50ada6afb5659/src/main/java/io/cryostat/net/OpenShiftAuthManager.java#L270-L288

It still has the create tokenreviews permission from the existing ClusterRole. So the user:info scope is not being used right now. Later on, if we change the TokenReview to an OpenShift user API query using the scoped token, the user:info scope will be necessary. I think that's probably best saved for another PR though.

Thanks for the clarification. I'll remove user:info from the backend request then.

ebaron · 2021-12-08T22:17:21Z

internal/controllers/common/resource_definitions/resource_definitions.go

+			Annotations: map[string]string{
+				"serviceaccounts.openshift.io/oauth-redirecturi.route":       "https://",
+				"serviceaccounts.openshift.io/oauth-redirectreference.route": string(ref),
+			},
 		},
 	}


Although it's probably harmless, I think we should only set these annotations when running on OpenShift. A Kubernetes user might be confused why there are OpenShift annotations pointing to a non-existent Route. You can pass r.IsOpenShift when calling this method like we do for NewDeploymentForCR.

ebaron · 2021-12-08T22:58:41Z

internal/controllers/common/resource_definitions/resource_definitions.go

+		sa := &corev1.ServiceAccount{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      cr.Name,
+				Namespace: cr.Namespace,
+				Labels: map[string]string{
+					"app": "cryostat",
+				},
+				Annotations: map[string]string{
+					"serviceaccounts.openshift.io/oauth-redirecturi.route":       "https://",
+					"serviceaccounts.openshift.io/oauth-redirectreference.route": string(ref),
+				},
+			},
+		}
+		return sa, nil
+	}


Just one more suggestion to reduce duplication, you could:

Create an empty annotations map

Put the OAuth annotations in the map inside the OpenShift branch

Return the ServiceAccount with annotations set to the map. In Kubernetes this will be empty, in OpenShift it'll contain the OAuth annotations

ebaron

I noticed one more thing, this new ClusterRole will need to be added to our operator bundle. Running make bundle should copy it to the necessary place.

ebaron · 2021-12-09T20:10:57Z

internal/controllers/common/resource_definitions/resource_definitions.go

+		}
+
+		annotations = map[string]string{
+			"serviceaccounts.openshift.io/oauth-redirecturi.route":       "https://",


I think perhaps we should remove this annotation. Right now the operator always creates Routes with some form of TLS enabled, but I think in the near future we may want to allow the user to customize the Routes we create like we do with Ingresses and now Services. In the interest of future-proofing this, I suggest we don't assume HTTPS here.

I tracked down the code that parses the route provided in the oauth-redirectreference.route annotation:
https://github.com/openshift/library-go/blob/7a65fdb398e28782ee1650959a5e0419121e97ae/pkg/oauth/oauthserviceaccountclient/oauthclientregistry.go#L418-L441

It will use HTTPS for the scheme, unless the Route has no TLS configuration, which sounds like what we'd want. We should be fine to not override this behaviour by forcing HTTPS.

ebaron

Looks good, nice work!

jan-law commented Nov 5, 2021

View reviewed changes

internal/controllers/common/resource_definitions/resource_definitions.go Outdated Show resolved Hide resolved

jan-law added the enhancement label Nov 5, 2021

jan-law requested a review from ebaron November 5, 2021 19:05

jan-law force-pushed the create-oauth-client branch from 974cef9 to 33fefbe Compare November 15, 2021 19:14

jan-law commented Nov 15, 2021

View reviewed changes

internal/controllers/common/resource_definitions/resource_definitions.go Outdated Show resolved Hide resolved

jan-law force-pushed the create-oauth-client branch 3 times, most recently from d466f01 to 468b012 Compare November 22, 2021 17:32

jan-law commented Nov 22, 2021

View reviewed changes

internal/controllers/common/resource_definitions/resource_definitions.go Outdated Show resolved Hide resolved

jan-law force-pushed the create-oauth-client branch 2 times, most recently from 380589a to 61e73c7 Compare December 1, 2021 19:47

jan-law marked this pull request as ready for review December 1, 2021 20:18

jan-law mentioned this pull request Dec 1, 2021

feat(auth): Redirect to OpenShift login on empty auth request cryostatio/cryostat-legacy#748

Merged

jan-law force-pushed the create-oauth-client branch from 61e73c7 to 4ef5b79 Compare December 6, 2021 23:38

ebaron reviewed Dec 7, 2021

View reviewed changes

jan-law marked this pull request as draft December 7, 2021 23:26

Janelle Law added 8 commits December 8, 2021 14:01

Add oauth client as service account

c3374d7

Set redirect uri dynamically

25cf21d

Reuse cryostat clusterRole for oauth permissions

8746029

Add oauth env vars

dca5008

Update tests

2371b2b

Update clusterRole permissions

ff9e518

Add tree API get permissions

62220fe

fixup! Add tree API get permissions

4e2d051

jan-law force-pushed the create-oauth-client branch from 95dd1b9 to 4047316 Compare December 8, 2021 19:01

Move oauth permissions to new clusterRole

e154aa1

jan-law force-pushed the create-oauth-client branch from 4047316 to e154aa1 Compare December 8, 2021 19:03

jan-law marked this pull request as ready for review December 8, 2021 19:06

ebaron reviewed Dec 8, 2021

View reviewed changes

Only add route to openshift sa

729efe2

ebaron reviewed Dec 8, 2021

View reviewed changes

fixup! Only add route to openshift sa

b9a5298

ebaron reviewed Dec 9, 2021

View reviewed changes

Add clusterRole to bundle

51f8112

ebaron reviewed Dec 9, 2021

View reviewed changes

Janelle Law added 2 commits December 9, 2021 15:55

Make TLS optional for routes

e45c52e

fixup! Make TLS optional for routes

b982722

ebaron approved these changes Dec 9, 2021

View reviewed changes

ebaron merged commit 30d2044 into cryostatio:main Dec 9, 2021

jan-law deleted the create-oauth-client branch December 10, 2021 16:12

ebaron added feat New feature or request and removed enhancement labels Apr 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): Add OAuth client to service account #292

feat(auth): Add OAuth client to service account #292

jan-law commented Nov 5, 2021 •

edited

Loading

ebaron commented Nov 22, 2021

jan-law commented Dec 1, 2021

ebaron Dec 7, 2021

jan-law Dec 7, 2021

jan-law Dec 8, 2021

ebaron Dec 8, 2021

jan-law Dec 8, 2021

ebaron Dec 8, 2021

jan-law Dec 8, 2021

ebaron Dec 8, 2021

ebaron Dec 8, 2021

ebaron left a comment

ebaron Dec 9, 2021

ebaron left a comment

feat(auth): Add OAuth client to service account #292

feat(auth): Add OAuth client to service account #292

Conversation

jan-law commented Nov 5, 2021 • edited Loading

ebaron commented Nov 22, 2021

jan-law commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebaron left a comment

Choose a reason for hiding this comment

jan-law commented Nov 5, 2021 •

edited

Loading