feat(openshift): add configuration for proxy SubjectAccessReview #132
Conversation
|
@ebaron one shortcoming so far is that this only allows for a single namespace/resource/verb tuple. The proxy supports supplying a list to this Another approach that seems easier to implement but messier for the configuration interface would be something like a |
|
Sorted it out - I made the new |
|
Last commit enables ex. |
charts/cryostat/values.yaml
Outdated
| tokenReview: | ||
| ## @param openshiftOauthProxy.tokenReview.group The OpenShift resource group that the TokenReview will be performed for | ||
| group: "" | ||
| ## @param openshiftOauthProxy.tokenReview.resource The OpenShift resource that the TokenReview will be performed for | ||
| resource: "pods/exec" | ||
| ## @param openshiftOauthProxy.tokenReview.verb The OpenShift resource verb that the TokenReview will be performed for | ||
| verb: "create" |
There was a problem hiding this comment.
Maybe we can add a doc link to: https://github.com/openshift/oauth-proxy/?tab=readme-ov-file#delegate-authentication-and-authorization-to-openshift-for-infrastructure to clarify the use of this value?
There was a problem hiding this comment.
The value of the flag is a JSON map of path prefixes to v1beta1.ResourceAttributes, and the longest path prefix is checked. If no path matches the request, authentication and authorization are skipped.
Should other options configurable? I suppose namespace should be at least configurable, otherwise "" (empty) means all namespaces, which might not be possible for regular users.
There was a problem hiding this comment.
I guess conceptually the token reviews should also be a list like the subject access reviews, and this list should actually be the list of namespaces that Cryostat is watching plus its installation namespace. This seems difficult to implement in the Helm chart at least, and is rather different from how we did access control in previous Cryostat versions, so for now I think it's enough to use the installation namespace for RBAC checks.
There was a problem hiding this comment.
Ahh makes sense! Also, I am a bit unsure of --openshift-delegate-urls supports key -> JSON array or must it be a single JSON object per path? Its rather odd if its just a single object.
I guess conceptually the token reviews should also be a list like the subject access reviews, and this list should actually be the list of namespaces that Cryostat is watching plus its installation namespace.
If JSON array is supported in token review, could we default its mapped values for each path prefix to SubjectAccessReview's rules and leave the token review config empty by default?
There was a problem hiding this comment.
It was a bit hard to follow but when I was reviewing its sources earlier it did look like it only expected a single object here, not an array. But I haven't actually tried it to see if it accepts it and works, so it's probably worth giving a shot - at least with a singleton array to start with.
If it works then I would actually think of using the token review to define the list of required permissions, and then the accessReview to simply assign that same list to each of the path prefixes. It seems like that would be easier to accomplish with the Helm chart templates and filters.
There was a problem hiding this comment.
If JSON array is supported in token review, could we default its mapped values for each path prefix to SubjectAccessReview's rules and leave the token review config empty by default?
If it works then I would actually think of using the token review to define the list of required permissions, and then the accessReview to simply assign that same list to each of the path prefixes. It seems like that would be easier to accomplish with the Helm chart templates and filters.
If JSON array is supported in token review, could we default its mapped values for each path prefix to SubjectAccessReview's rules and leave the token review config empty by default?
Oh sorry haha I meant to say the same you explained there but with tokenReview still available but left as null in values.yaml. But it does sound simpler to just copy from accessReview.
There was a problem hiding this comment.
I tried this:
diff --git a/charts/cryostat/templates/openshiftOauthProxy.tpl b/charts/cryostat/templates/openshiftOauthProxy.tpl
index 80a982d..df926b7 100644
--- a/charts/cryostat/templates/openshiftOauthProxy.tpl
+++ b/charts/cryostat/templates/openshiftOauthProxy.tpl
@@ -17,7 +17,7 @@
- --tls-key=/etc/tls/private/tls.key
- --proxy-prefix=/oauth2
- --openshift-sar={{ tpl ( .Values.openshiftOauthProxy.accessReview | toJson ) . }}
- - --openshift-delegate-urls={"/api":{{ tpl ( .Values.openshiftOauthProxy.tokenReview | toJson ) . }}, "/storage":{{ tpl ( .Values.openshiftOauthProxy.tokenReview | toJson ) . }}, "/grafana":{{ tpl ( .Values.openshiftOauthProxy.tokenReview | toJson ) . }} }
+ - --openshift-delegate-urls={"/api":[{{ tpl ( .Values.openshiftOauthProxy.tokenReview | toJson ) . }}], "/storage":[{{ tpl ( .Values.openshiftOauthProxy.tokenReview | toJson ) . }}], "/grafana":[{{ tpl ( .Values.openshiftOauthProxy.tokenReview | toJson ) . }}] }
{{- if .Values.authentication.basicAuth.enabled }}
- --htpasswd-file=/etc/openshift_oauth_proxy/basicauth/{{ .Values.authentication.basicAuth.filename }}
{{- end }}and it broke the auth proxy:
$ oc logs -f pod/cryostat-68ff4cffc9-kltmp
Defaulted container "cryostat-authproxy" out of: cryostat-authproxy, cryostat, cryostat-db, cryostat-storage, cryostat-grafana, cryostat-jfr-datasource
2024/04/19 20:46:25 main.go:145: resources must be a JSON map of paths to authorizationv1beta1.ResourceAttributes: json: cannot unmarshal array into Go value of type v1.ResourceAttributes
There was a problem hiding this comment.
Ahh that's unfortunate :(( I think we would have to keep the current implementation meanwhile then...
| @@ -16,6 +16,8 @@ | |||
| - --tls-cert=/etc/tls/private/tls.crt | |||
| - --tls-key=/etc/tls/private/tls.key | |||
| - --proxy-prefix=/oauth2 | |||
| - --openshift-sar={{ tpl ( .Values.openshiftOauthProxy.accessReview | toJson ) . }} | |||
| - --openshift-delegate-urls={"/":{{ .Values.openshiftOauthProxy.tokenReview | toJson }}} | |||
There was a problem hiding this comment.
Would it make sense to restrict the path prefix to /api since its usage its mostly allow non-browser access or access from service account?
There was a problem hiding this comment.
I was thinking about that, but this would mean that it's only authorizing on requests that will be passed to the Cryostat upstream, and not to requests that go to Grafana or S3 or any other future containers that may be configured to be reachable through the proxy. I guess these could all have different token reviews, but it seemed simplest to just keep them everything guarded equally. If there is a case for expanding the configuration options later we can always add that.
There was a problem hiding this comment.
Or, another option would be to use the same token review for the paths /api, /storage, and /grafana. This way all three current upstream containers are guarded with the same RBAC, but notable /health (which also goes to the upstream Cryostat container) would become unguarded, which seems like it makes sense.
There was a problem hiding this comment.
Sounds good to me! It would be nice to have the key as regular expression but not sure if that's supported in ocp-oauth :D
There was a problem hiding this comment.
I was looking at the sources earlier and it doesn't look it supports anything other than string prefixes, unfortunately.
There was a problem hiding this comment.
Ahh opps.. I guess I can have a look and see if I can help them on that haha :D
There was a problem hiding this comment.
The repo activity is very dead and looks like it is on maintenance mode, so I'm not sure if that's worth the time. Not at least without talking to someone from that team first and verifying if they'd actually accept such a contribution upstream.
There was a problem hiding this comment.
Do you who I should contact? I am unsure who are the maintainers there :D Probably not anytime soon tho until I finish some tasks here :D
There was a problem hiding this comment.
https://github.com/openshift/oauth-proxy/commits?author=stlaz would be the one to contact, I think.
There was a problem hiding this comment.
Okay I will try contacting them sometimes soon! Thank youu!!
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: {{ include "cryostat.fullname" . }} | ||
| labels: | ||
| {{- include "cryostat.labels" . | nindent 4 }} | ||
| rules: | ||
| - apiGroups: | ||
| - "authorization.k8s.io" | ||
| resources: | ||
| - subjectaccessreviews | ||
| verbs: | ||
| - create | ||
| - apiGroups: | ||
| - "authentication.k8s.io" | ||
| resources: | ||
| - tokenreviews | ||
| verbs: | ||
| - create |
There was a problem hiding this comment.
Can we use the built-in cluterrole system:auth-delegator on openshift since it specifies the same rules? Maybe we can add a check if auth.openshift.enabled too?
$ oc get clusterrole/system:auth-delegator -o yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
annotations:
rbac.authorization.kubernetes.io/autoupdate: "true"
creationTimestamp: "2023-08-24T02:53:01Z"
labels:
kubernetes.io/bootstrapping: rbac-defaults
name: system:auth-delegator
resourceVersion: "101"
uid: 55530dfd-424d-4d17-805c-b3f9b4bd2ae3
rules:
- apiGroups:
- authentication.k8s.io
resources:
- tokenreviews
verbs:
- create
- apiGroups:
- authorization.k8s.io
resources:
- subjectaccessreviews
verbs:
- createThere was a problem hiding this comment.
Good ideas, I'll try that out.
|
Fun and easy test to validate all this. By default:
Giving
Now the user has access:
If I delete the
And then log out and back in again:
|
andrewazores
force-pushed
the
openshift-sar
branch
from
7f64c1a to
a6c74fd
Compare
charts/cryostat/README.md
Outdated
| | `openshiftOauthProxy.subjectAccessReview[0].group` | The OpenShift resource group that the SubjectAccessReview will be performed for | `""` | | ||
| | `openshiftOauthProxy.subjectAccessReview[0].resource` | The OpenShift resource that the SubjectAccessReview will be performed for | `pods` | | ||
| | `openshiftOauthProxy.subjectAccessReview[0].subresource` | The OpenShift subresource that the SubjectAccessReview will be performed for | `exec` | | ||
| | `openshiftOauthProxy.subjectAccessReview[0].name` | The OpenShift resource name that the SubjectAccessReview will be performed for | `""` | | ||
| | `openshiftOauthProxy.subjectAccessReview[0].namespace` | The OpenShift Namespace that the SubjectAccessReview will be performed for. | `{{ .Release.Namespace }}` | | ||
| | `openshiftOauthProxy.subjectAccessReview[0].verb` | The OpenShift resource verb that the SubjectAccessReview will be performed for | `create` | | ||
| | `openshiftOauthProxy.subjectAccessReview[0].version` | The OpenShift resource version that the SubjectAccessReview will be performed for | `""` | | ||
| | `openshiftOauthProxy.tokenAccessReview.group` | The OpenShift resource group that the TokenAccessReview will be performed for. See https://github.com/openshift/oauth-proxy/?tab=readme-ov-file#delegate-authentication-and-authorization-to-openshift-for-infrastructure | `""` | | ||
| | `openshiftOauthProxy.tokenAccessReview.resource` | The OpenShift resource that the TokenAccessReview will be performed for. | `pods` | | ||
| | `openshiftOauthProxy.tokenAccessReview.subresource` | The OpenShift resource that the TokenAccessReview will be performed for. | `exec` | | ||
| | `openshiftOauthProxy.tokenAccessReview.name` | The OpenShift resource name that the TokenAccessReview will be performed for. | `""` | | ||
| | `openshiftOauthProxy.tokenAccessReview.namespace` | The OpenShift namespace that the TokenAccessReview will be performed for. | `{{ .Release.Namespace }}` | | ||
| | `openshiftOauthProxy.tokenAccessReview.verb` | The OpenShift resource name that the TokenAccessReview will be performed for. | `create` | | ||
| | `openshiftOauthProxy.tokenAccessReview.version` | The OpenShift resource version that the TokenAccessReview will be performed for. | `""` | |
There was a problem hiding this comment.
Should we combine the tokenAccessReview into subjectAccessReview to match the operator's implementation?
https://github.com/cryostatio/cryostat-operator/blob/b300689a645e3741ccd5468e09acf20b5b9fb4d2/api/v1beta2/cryostat_types.go#L484-L496
…proxy to handle requests presenting 'Authorization: Bearer' headers
andrewazores
force-pushed
the
openshift-sar
branch
from
948da72 to
0714655
Compare
| - --openshift-sar=[{{ tpl ( .Values.openshiftOauthProxy.accessReview | toJson ) . }}] | ||
| - --openshift-delegate-urls={"/":{{ tpl ( .Values.openshiftOauthProxy.accessReview | toJson ) . }}} |
There was a problem hiding this comment.
Do we want to make this bit optional? This would allow OpenShift OAuth to work for namespace admins (who can't create ClusterRoleBindings), just without an SAR.
There was a problem hiding this comment.
Good idea. I'll try it out.
Based on #124
See https://github.com/openshift/oauth-proxy/?tab=readme-ov-file#limiting-access-to-users
Adds configuration parameters to customize the
SubjectAccessReviewperformed by the OpenShift OAuth Proxy. All requests going through the proxy via OpenShift SSO (not htpasswd Basic ones, if they are enabled) must be able to pass this SAR, which the proxy does before forwarding the request upstream. The default setting is to require clients have the ability tocreatepods/execwithin the Helm's.Release.Namespace.