feat(cryostat): re-implement basic auth via oauth2-proxy htpasswd config #118

aali309 · 2024-01-18T21:20:03Z

fixes: #116

charts/cryostat/templates/auth_proxy.yaml

andrewazores · 2024-01-23T23:18:43Z

Not sure why it's showing so many conflicts - just try a git fetch upstream && git merge rebase -i upstream/main and drop any commits from my PR that git doesn't realize are already incorporated in the base branch.

.github/workflows/helm-release.yml

charts/cryostat/templates/deployment.yaml

charts/cryostat/values.yaml

charts/cryostat/templates/deployment.yaml

charts/cryostat/values.yaml

charts/cryostat/templates/deployment.yaml

andrewazores · 2024-01-25T13:59:52Z

Rebase on top of upstream/cryostat3 again please - I just updated that to be up to date with main.

andrewazores · 2024-02-08T15:48:08Z

Okay, so now that we've figured out how to get it deployable and running again... back to reviewing the important changes from this PR.

charts/cryostat/auth_proxy_htpasswd

charts/cryostat/templates/deployment.yaml

charts/cryostat/templates/alpha_config.yaml

andrewazores · 2024-02-08T17:41:42Z

I think with my latest changes this is more or less working. When you open the route URL you hit the oauth2-proxy login page with the htpasswd login form, just like in the cryostat3 smoketest. When you use user:pass to log in then you see the Cryostat UI and can interact with it as usual.

Next steps for you to tackle, please:

Fix up the 2 auth2_proxy.yaml. In my commit it's just using a hardcoded value representing user:pass. In your previous commits it was trying to use a hardcoded file to use the same values. We don't want to hardcode the values at all - these must be configured and supplied by the user. This means that the user must create their own Secret with their own contents (ie user:pass or something else, with the correct encoding etc already). Then the user passes this via values.yaml .authentication.basicAuth configuration.
If values.yaml .authentication.basicAuth.enabled = false then set the env var OAUTH2_PROXY_SKIP_AUTH_ROUTES=.* on the authproxy container. This way the auth proxy is always in place, but if the user does not enable authentication, then the proxy just passes requests through without requiring the user to authenticate.
values.yaml section for oauth2Proxy should not have the htpasswdFile property - I think this is doing the same thing that .authentication.basicAuth.secretName/.authentication.basicAuth.filename is doing. Also, .oauth2Proxy.alphaConfig looks like it should probably be removed and always hardcoded - I can't see that the user would need to be able to change these configurations at this level of granularity.

For separate PR(s):
4. env var STORAGE_EXT_URL needs to be set on the Cryostat container.
5. Later when we add support for deploying openshift-oauth-proxy in place of oauth2-proxy, the values.yaml .oauth2Proxy.enabled configuration should also be replaced with something else like .core.openshift-integration.enabled. This would switch on both openshift-oauth-proxy and I think should also replace .core.route.enabled. Then the user has these options: 1. leave .authentication.basic.enabled and .core.openshift-integration.enabled off, in which case they get oauth2-proxy with no authentication. 2. turn on .authentication.basic.enabled only, in which case they get what this PR does - oauth2-proxy with htpasswd. 3. turn on .core.openshift-integration.enabled only, in which case they get openshift-oauth-proxy with only OpenShift SSO and no htpasswd, 4. turn on both, in which case they get openshift-oauth-proxy with both OpenShift SSO and htpasswd.

charts/cryostat/templates/auth2_proxy.yaml

charts/cryostat/templates/deployment.yaml

andrewazores · 2024-02-09T17:20:19Z

For example, I created this secret manually:

apiVersion: v1
data:
  htpasswd: dXNlcjokMnkkMDUkLnAxLzY4b0JXS1gxRmhBdFpPQXJZT29Ob01xc0I0eXVVTlhHT2VTQVRIUHEzZ2VLcUVhWlM=
kind: Secret
metadata:
  creationTimestamp: "2024-02-09T17:07:49Z"
  name: basicauth
  namespace: cryostat3
  resourceVersion: "75991"
  uid: 16eab9c1-8dd3-4b64-b544-cbb4d52cccba
type: Opaque

This is still just user:pass like before. The data part can be generated using a command like htpasswd -nbB user pass | base64 so you can try this out with other usernames/passwords, or with multiple permissible logins, etc. Try this:

$ htpasswd -nbB user pass >> htpasswd
$ htpasswd -nbB atif atifspassword >> htpasswd
$ oc create secret generic basicauth --from-file htpasswd=./htpasswd # no need to use `base64` command if you create from file like this

and I applied this patch on top of this branch:

diff --git a/charts/cryostat/templates/auth2_proxy.yaml b/charts/cryostat/templates/auth2_proxy.yaml
deleted file mode 100644
index e49b4b1..0000000
--- a/charts/cryostat/templates/auth2_proxy.yaml
+++ /dev/null
@@ -1,6 +0,0 @@
-apiVersion: v1
-kind: Secret
-metadata:
-  name: {{ .Release.Name }}-auth-proxy-secret
-data:
-    auth_proxy_htpasswd: {{ .Values.authentication.basicAuth.secretName | b64enc | quote }}
diff --git a/charts/cryostat/templates/deployment.yaml b/charts/cryostat/templates/deployment.yaml
index 88536bd..d420158 100644
--- a/charts/cryostat/templates/deployment.yaml
+++ b/charts/cryostat/templates/deployment.yaml
@@ -208,21 +208,22 @@ spec:
               value: {{ include "cryostat.cookieSecret" . }}
             - name: OAUTH2_PROXY_EMAIL_DOMAINS
               value: "*"
+            {{- if .Values.authentication.basicAuth.enabled }}
             - name: OAUTH2_PROXY_HTPASSWD_USER_GROUP
               value: write
+            - name: OAUTH2_PROXY_HTPASSWD_FILE
+              value: /etc/oauth2_proxy/basicauth/{{ .Values.authentication.basicAuth.filename }}
+            {{- end }}
             {{- if not .Values.authentication.basicAuth.enabled }}
             - name: OAUTH2_PROXY_SKIP_AUTH_ROUTES
               value: ".*"
-          {{- end }}
+            {{- end }}
           volumeMounts:
-            - name: auth-proxy-config
-              mountPath: /etc/oauth2_proxy
-              readOnly: true
             - name: alpha-config
               mountPath: /etc/oauth2_proxy/alpha_config
             {{- if .Values.authentication.basicAuth.enabled }}
-            - name: basic-auth-secret
-              mountPath: /etc/oauth2_proxy/basic-auth-secret
+            - name: {{ .Release.Name }}-htpasswd
+              mountPath: /etc/oauth2_proxy/basicauth
               readOnly: true
             {{- end }}
         - name: {{ printf "%s-%s" .Chart.Name "grafana" }}
@@ -291,11 +292,8 @@ spec:
       - name: alpha-config
         configMap:
           name: {{ .Release.Name }}-alpha-config
-      - name: auth-proxy-config
-        secret:
-          secretName: {{ .Release.Name }}-auth-proxy-secret
       {{- if .Values.authentication.basicAuth.enabled }}
-      - name: basic-auth-secret
+      - name: {{ .Release.Name }}-htpasswd
         secret:
           secretName: {{ .Values.authentication.basicAuth.secretName }}
       {{- end }}

With this, I can do:

$ helm install cryostat --set pvc.enabled=true --set core.route.enabled=true --set authentication.basicAuth.enabled=false --set authentication.basicAuth.secretName=basicauth --set authentication.basicAuth.filename=htpasswd ./charts/cryostat

and get a deployment that has an authproxy set up but which passes all requests without authentication. Simply changing that to authentication.basicAuth.enabled=true, like this:

$ helm install cryostat --set pvc.enabled=true --set core.route.enabled=true --set authentication.basicAuth.enabled=true --set authentication.basicAuth.secretName=basicauth --set authentication.basicAuth.filename=htpasswd ./charts/cryostat

gets me a deployment that uses the Secret I created to perform basic auth on the proxy.

…er-supplied htpasswd

aali309 · 2024-02-09T17:25:23Z

For example, I created this secret manually:

apiVersion: v1
data:
  htpasswd: dXNlcjokMnkkMDUkLnAxLzY4b0JXS1gxRmhBdFpPQXJZT29Ob01xc0I0eXVVTlhHT2VTQVRIUHEzZ2VLcUVhWlM=
kind: Secret
metadata:
  creationTimestamp: "2024-02-09T17:07:49Z"
  name: basicauth
  namespace: cryostat3
  resourceVersion: "75991"
  uid: 16eab9c1-8dd3-4b64-b544-cbb4d52cccba
type: Opaque

This is still just user:pass like before.

and I applied this patch on top of this branch:

diff --git a/charts/cryostat/templates/auth2_proxy.yaml b/charts/cryostat/templates/auth2_proxy.yaml
deleted file mode 100644
index e49b4b1..0000000
--- a/charts/cryostat/templates/auth2_proxy.yaml
+++ /dev/null
@@ -1,6 +0,0 @@
-apiVersion: v1
-kind: Secret
-metadata:
-  name: {{ .Release.Name }}-auth-proxy-secret
-data:
-    auth_proxy_htpasswd: {{ .Values.authentication.basicAuth.secretName | b64enc | quote }}
diff --git a/charts/cryostat/templates/deployment.yaml b/charts/cryostat/templates/deployment.yaml
index 88536bd..d420158 100644
--- a/charts/cryostat/templates/deployment.yaml
+++ b/charts/cryostat/templates/deployment.yaml
@@ -208,21 +208,22 @@ spec:
               value: {{ include "cryostat.cookieSecret" . }}
             - name: OAUTH2_PROXY_EMAIL_DOMAINS
               value: "*"
+            {{- if .Values.authentication.basicAuth.enabled }}
             - name: OAUTH2_PROXY_HTPASSWD_USER_GROUP
               value: write
+            - name: OAUTH2_PROXY_HTPASSWD_FILE
+              value: /etc/oauth2_proxy/basicauth/{{ .Values.authentication.basicAuth.filename }}
+            {{- end }}
             {{- if not .Values.authentication.basicAuth.enabled }}
             - name: OAUTH2_PROXY_SKIP_AUTH_ROUTES
               value: ".*"
-          {{- end }}
+            {{- end }}
           volumeMounts:
-            - name: auth-proxy-config
-              mountPath: /etc/oauth2_proxy
-              readOnly: true
             - name: alpha-config
               mountPath: /etc/oauth2_proxy/alpha_config
             {{- if .Values.authentication.basicAuth.enabled }}
-            - name: basic-auth-secret
-              mountPath: /etc/oauth2_proxy/basic-auth-secret
+            - name: {{ .Release.Name }}-htpasswd
+              mountPath: /etc/oauth2_proxy/basicauth
               readOnly: true
             {{- end }}
         - name: {{ printf "%s-%s" .Chart.Name "grafana" }}
@@ -291,11 +292,8 @@ spec:
       - name: alpha-config
         configMap:
           name: {{ .Release.Name }}-alpha-config
-      - name: auth-proxy-config
-        secret:
-          secretName: {{ .Release.Name }}-auth-proxy-secret
       {{- if .Values.authentication.basicAuth.enabled }}
-      - name: basic-auth-secret
+      - name: {{ .Release.Name }}-htpasswd
         secret:
           secretName: {{ .Values.authentication.basicAuth.secretName }}
       {{- end }}

With this, I can do:

$ helm install cryostat --set pvc.enabled=true --set core.route.enabled=true --set authentication.basicAuth.enabled=false --set authentication.basicAuth.secretName=basicauth --set authentication.basicAuth.filename=htpasswd ./charts/cryostat

and get a deployment that has an authproxy set up but which passes all requests without authentication. Simply changing that to authentication.basicAuth.enabled=true, like this:

$ helm install cryostat --set pvc.enabled=true --set core.route.enabled=true --set authentication.basicAuth.enabled=true --set authentication.basicAuth.secretName=basicauth --set authentication.basicAuth.filename=htpasswd ./charts/cryostat

gets me a deployment that uses the Secret I created to perform basic auth on the proxy.

yes I have done something similar...
i get this error from storage as before:
cryostat/cryostat-storage:latest" in 209.372526ms Warning Failed 70s kubelet Error: container has runAsNonRoot and image will run as root (pod: "cryostat-688d4f9d5c-582w5_default(f83c8023-a0df-4385-a9a7-9abb78b24910)", container: cryostat-storage)

but runs well on OpenShift

andrewazores · 2024-02-09T17:29:27Z

Are you in the default namespace/project again? I don't think any of the changes made here should have any effect on whether the root user is used.

aali309 · 2024-02-09T17:47:43Z

Looks good now on my side too.
when basicAuth is enabled and no secret defined
Error: INSTALLATION FAILED: Deployment.apps "cryostat" is invalid: [spec.template.spec.volumes[2].secret.secretName: Required value, spec.template.spec.containers[3].volumeMounts[1].name: Not found: "cryostat-htpasswd"]
which makes sense according to what you were telling me earlier.
when disabled no authentication is required.

andrewazores

I think this looks good now, but let's wait for @ebaron to be back and give it a look-over too.

ebaron

This looks mostly good, but I think there's still the odd bug. When I tested using the default ClusterIP service type, I didn't get directed to the OAuth login page, but to Cryostat itself.

Here's how I tested:

$ htpasswd -nbB foo bar > /tmp/htpasswd
$ oc create secret generic htpasswd --from-file=htpasswd=/tmp/htpasswd
$ helm install cryostat ./charts/cryostat/ --set authentication.basicAuth.enabled=true,authentication.basicAuth.secretName=htpasswd,authentication.basicAuth.filename=htpasswd

This is the output of the NOTES:

Tell Cryostat how to serve external traffic:

export POD_NAME=$(kubectl get pods -n helm-test -l "app.kubernetes.io/name=cryostat,app.kubernetes.io/instance=cryostat"  --sort-by=.metadata.creationTimestamp -o jsonpath="{.items[-1:].metadata.name}")
export CONTAINER_PORT=$(kubectl get pod -n helm-test $POD_NAME -o jsonpath="{.spec.containers[0].ports[0].containerPort}")
export GRAFANA_CONTAINER_PORT=$(kubectl get pod -n helm-test $POD_NAME -o jsonpath="{.spec.containers[1].ports[0].containerPort}")
kubectl -n helm-test set env deploy --containers=cryostat cryostat CRYOSTAT_WEB_HOST=127.0.0.1 CRYOSTAT_EXT_WEB_PORT=8080 GRAFANA_DASHBOARD_URL=http://127.0.0.1:$GRAFANA_CONTAINER_PORT GRAFANA_DASHBOARD_EXT_URL=http://127.0.0.1:8081

Forward local ports to the application's pod:

export POD_NAME=$(kubectl get pods -n helm-test -l "app.kubernetes.io/name=cryostat,app.kubernetes.io/instance=cryostat"  --sort-by=.metadata.creationTimestamp -o jsonpath="{.items[-1:].metadata.name}")
kubectl -n helm-test wait --for=condition=available --timeout=60s deploy/cryostat
kubectl -n helm-test port-forward $POD_NAME 8080:$CONTAINER_PORT 8081:$GRAFANA_CONTAINER_PORT

Visit the Cryostat application at:

http://127.0.0.1:8080

I think in this case the CONTAINER_PORT environment variable is wrong. This should point to the oauth-proxy's container port and not Cryostat's container port.

Also, should Cryostat be binding to localhost instead of 0.0.0.0 now?

andrewazores · 2024-02-13T22:05:14Z

I'm working on that ClusterIP setup and related things in #122 . I think the Route and ClusterIP setups in that draft are mostly working but I still need to figure out the other options (Ingress etc.)

I think I only tested this PR with core.route.enabled=true since it was just a bit simpler and I was using crc anyway.

andrewazores · 2024-02-13T22:06:20Z

Also, should Cryostat be binding to localhost instead of 0.0.0.0 now?

That's a good call, I'll put that change here since it's just a oneliner.

The storage container also binds on 0.0.0.0 right now. I tried localhost and 127.0.0.1 earlier and that broke it somehow, but I didn't really understand why. That container actually runs at least three different server processes of different roles internally, so I guess it has something to do with that. Worth investigating more.

ebaron · 2024-02-13T22:24:06Z

Looks good. The remaining issues can be tackled in #122. Good work @aali309!

aali309 added feat New feature or request safe-to-test labels Jan 18, 2024

aali309 requested a review from andrewazores January 18, 2024 21:20

andrewazores reviewed Jan 18, 2024

View reviewed changes

charts/cryostat/templates/auth_proxy.yaml Outdated Show resolved Hide resolved

andrewazores changed the base branch from main to cryostat3 January 18, 2024 21:30

aali309 force-pushed the oauth2-proxy branch 2 times, most recently from 49fd8c7 to 8c246a8 Compare January 19, 2024 01:45

aali309 closed this Jan 22, 2024

aali309 reopened this Jan 22, 2024

aali309 force-pushed the oauth2-proxy branch from d48b089 to d1cdc2a Compare January 24, 2024 02:18