Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(auth): optionally deploy OpenShift OAuth Proxy #127

Merged
merged 8 commits into from
Apr 10, 2024

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Mar 18, 2024

Replaces #126
Based on #124
Fixes #117

Not handled here: RBAC. This puts the OpenShift OAuth Proxy in front of the other containers in the Pod, but it will allow any OpenShift account (or Basic credentials if configured additionally) through. Restricting access to OpenShift accounts with particular roles/permissions is still TODO - I wonder if the required set of permissions should just be hardcoded or if this should be something that the deploying user can configure as well.

@andrewazores
Copy link
Member Author

Not sure if I'm missing something obvious but with this setup, the OpenShift login redirect flow works and the user can get into the Cryostat Web UI, load all the assets, and make normal HTTP requests. However, the WebSocket connection to the server fails with an HTTP 500. As far as I can tell that's coming from the OpenShift OAuth Proxy itself, but it doesn't provide anything in the logs more detailed than simply the access log line indicating that it saw the request and responded with a 500 within 1ms.

@andrewazores
Copy link
Member Author

Example of such an access log line:

10.217.0.1 - [email protected] [18/Mar/2024:16:32:04 +0000] cryostat-helm-openshift.apps-crc.testing GET localhost:8181 "/api/notifications" HTTP/1.1 "Mozilla/5.0 (X11; Linux x86_64; rv:123.0) Gecko/20100101 Firefox/123.0" 500 16 0.001

@andrewazores
Copy link
Member Author

Screenshot_2024-03-18_15-24-08

@andrewazores
Copy link
Member Author

I also tried with a rebuild of the oauth-proxy using golang 1.22.1, but it looks the exact same.

$ helm install cryostat --set authentication.openshift.enabled=true --set core.route.enabled=true --set openshiftOauthProxy.image.repository=quay.io/andrewazores/openshift-oauth-proxy --set openshiftOauthProxy.image.tag=test-14 ./charts/cryostat/

@andrewazores andrewazores marked this pull request as ready for review March 18, 2024 20:10
@andrewazores
Copy link
Member Author

Other than the WebSocket bug, which I'm pretty well convinced is another openshift-oauth-proxy bug, I think this PR is basically complete.

@andrewazores
Copy link
Member Author

If I apply the same interceptor trick to the oauth-proxy rebuild that I did here:

openshift/oauth-proxy#272 (comment)

this causes the proxy to simply pass all requests without actually requiring any authz (no OpenShift redirect etc.), and it also allows the frontend to properly establish its WebSocket connection with the backend via the proxy. That seems to confirm that this is a proxy bug.

@andrewazores andrewazores requested a review from ebaron March 18, 2024 20:16
@andrewazores
Copy link
Member Author

Aha: openshift/oauth-proxy#157

Let me try this.

@andrewazores
Copy link
Member Author

Ha. That's incredibly silly.

Screenshot_2024-03-18_16-24-06

@andrewazores
Copy link
Member Author

andrewazores commented Mar 18, 2024

@ebaron I remember discussing at some point before an idea about unifying the OpenShift SSO support and Route support into one "OpenShift integration" flag, or more generally something like a platform-integration: 'openshift' value. Do you think that would be worthwhile, and if so should I go ahead and do that in this PR? Right now these two features are split into basic.route.enabled and authentication.openshift.enabled.

@@ -300,5 +270,11 @@ spec:
{{- if .Values.authentication.basicAuth.enabled }}
- name: {{ .Release.Name }}-htpasswd
secret:
defaultMode: 420
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 0644 isn't it? Should we have something like 0440 instead?

capabilities:
drop:
- ALL
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this # a typo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, looks like it, whoops.

@ebaron
Copy link
Member

ebaron commented Apr 9, 2024

@ebaron I remember discussing at some point before an idea about unifying the OpenShift SSO support and Route support into one "OpenShift integration" flag, or more generally something like a platform-integration: 'openshift' value. Do you think that would be worthwhile, and if so should I go ahead and do that in this PR? Right now these two features are split into basic.route.enabled and authentication.openshift.enabled.

I think leaving them separate is fine for now. When releasing downstream, we can default them both to true. I imagine the downstream release will typically be how this Helm Chart is used on OpenShift.

Restricting access to OpenShift accounts with particular roles/permissions is still TODO - I wonder if the required set of permissions should just be hardcoded or if this should be something that the deploying user can configure as well.

It seems like the OAuth proxy expects a JSON string argument with the permissions. We could just define a default and allow the user to override it with their own JSON string. Not the most user-friendly, but the alternative I can think of is supplying a Role or ClusterRole, and at least the latter requires elevated permissions to do.

@andrewazores
Copy link
Member Author

andrewazores commented Apr 9, 2024

$ cat htpasswd.conf 
user:$2y$05$Ju.w7ibNuQ/xaAfKH74lJOJ5xqZbONaNWf7HnrjQuZP.4nhLrr0uS

$ oc create secret generic basicauth --from-file=htpasswd.conf
secret/basicauth created

$ helm install cryostat --set authentication.openshift.enabled=true --set core.route.enabled=true --set openshiftOauthProxy.image.repository=quay.io/andrewazores/openshift-oauth-proxy --set openshiftOauthProxy.image.tag=test-14 --set authentication.basicAuth.enabled=true --set authentication.basicAuth.secretName=basicauth --set authentication.basicAuth.filename=htpasswd.conf ./charts/cryostat/

With this, both OpenShift SSO and htpasswd Basic (user:pass) login seem to work just fine. This is after I updated the htpasswd permission to 0440 as well.

image

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@andrewazores andrewazores merged commit 76a89b1 into cryostatio:cryostat3 Apr 10, 2024
2 checks passed
@andrewazores andrewazores deleted the cryostat3-openshift branch April 10, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants