-
Notifications
You must be signed in to change notification settings - Fork 13
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(oauth2proxy): enable self-signed TLS cert #210
base: main
Are you sure you want to change the base?
Conversation
ec5c114
to
fbc760e
Compare
fbc760e
to
f6324eb
Compare
charts/cryostat/templates/NOTES.txt
Outdated
@@ -59,6 +63,6 @@ | |||
{{- else if contains "LoadBalancer" .Values.core.service.type }} | |||
echo http://$SERVICE_IP:{{ .Values.core.service.httpPort }} | |||
{{- else if contains "ClusterIP" .Values.core.service.type }} | |||
http://localhost:8080 | |||
{{ ternary "https" "http" (or .Values.authentication.openshift.enabled .Values.oauth2Proxy.service.tls.selfSigned.enabled) }}://localhost:{{ ternary "8443" "8080" (or .Values.authentication.openshift.enabled .Values.oauth2Proxy.service.tls.selfSigned.enabled) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since (or .Values.authentication.openshift.enabled .Values.oauth2Proxy.service.tls.selfSigned.enabled)
is being in many places, maybe we could refactor it into a helper named template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried this, but I just get complaints from Helm that the include
d template has type string
instead of bool
:/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried this, but I just get complaints from Helm that the included template has type string instead of bool :/
I guess we can return string "true"
/"false"
and do string compare? Or perhaps just return the protocol https
/http
? What do you think?
{{- if .Values.oauth2Proxy.service.tls.selfSigned.enabled }} | ||
SecureBindAddress: https://0.0.0.0:8443 | ||
{{- end}} | ||
TLS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be wrapped in the if block above?
charts/cryostat/values.yaml
Outdated
service: | ||
tls: | ||
selfSigned: | ||
## @param oauth2Proxy.service.tls.selfSigned.enabled Whether a self-signed TLS certificate for oauth2-proxy HTTPS is generated and used. | ||
enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be like below (i.e. removing service:
)?
tls:
selfSigned:
## @param oauth2Proxy.service.tls.selfSigned.enabled Whether a self-signed TLS certificate for oauth2-proxy HTTPS is generated and used.
enabled: false
@@ -5,6 +5,7 @@ metadata: | |||
labels: | |||
{{- include "cryostat.labels" . | nindent 4 }} | |||
app.kubernetes.io/component: test-grafana-connection | |||
helm-test: cryostat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, are there some reasons to add this label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make cleanup of the test Pods easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, how about using having standard format for cryostat-specific label? Like below or similar options:
charts.cryostat.io/role: helm-test
@@ -0,0 +1,15 @@ | |||
{{- if (and (not (.Values.authentication.openshift).enabled) (.Values.oauth2Proxy.service.tls.selfSigned.enabled)) }} | |||
{{- $fullName := include "cryostat.fullname" . }} | |||
{{- $cert := genSelfSignedCert $fullName nil nil 365 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: This means the certificate expires after a year right? This means the users have to rotate the certificate themselves?
Any thoughts about depending on cert-manager
to manage certificates like on the operator side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, it'll expire after a year. Users have to rotate it themselves or figure out something else automated. I don't really want to get into adding an external dependency to the Helm chart, since part of the reason for the chart to exist is for users who can't use the Operator, ex. because they don't have full admin control of the cluster or whatever else. If the user needs TLS but can't install the Operator, then this gives them at least something to work with as a starting point, and then they can build whatever other automation they need on top to suit their particular deployment environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Thanks for explaining! I think a quick note in README about this would be helpful :D
Working to rebase atop #206 now. |
6743730
to
cfb739e
Compare
Related to #168
Depends on #211 - that change is higher priority, and will conflict with this one since the port-forward post-install note will need to be updated for the new port names
Uses a Helm function to generate a self-signed cert, which is used for TLS on the oauth2-proxy. In the case that
authentication.openshift.enabled=true
is set, then the openshift-oauth-proxy is used instead and is presumably deployed on OpenShift, which has the serving-cert feature instead. If that value is not set, then the feature in this PR enables similar behaviour for the oauth2-proxy (whether deployed on OpenShift or not) so that the proxy only exposes HTTPS. This is disabled by default, and the post-installation notes are updated to instruct the user to port-forward to the HTTPS port and open that URL to find the Cryostat UI if they do enable it.I intend to follow up later on with similar PRs to enable self-signed TLS certs on the other components (db, storage) so that communications are always encrypted. It is also possible to create a self-signed CA, issue certs from that CA, and add that CA to a truststore. It's probably also possible to create the certs so that hostname verification of components works.
To test:
helm install mycryostat ./charts/cryostat
http://localhost:8080
port-forward. This should get you the Cryostat Web UI.helm uninstall mycryostat
helm install --set oauth2Proxy.service.tls.selfSigned.enabled=true mycryostat ./charts/cryostat
https://localhost:8443
port-forward. This should get you the Cryostat Web UI. You will likely need to accept a self-signed cert warning from your browser.