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

Fix handling of SSL #277

Merged
merged 6 commits into from
Jun 5, 2023
Merged

Fix handling of SSL #277

merged 6 commits into from
Jun 5, 2023

Conversation

ca-scribner
Copy link
Contributor

@ca-scribner ca-scribner commented May 29, 2023

This PR updates the EnvoyFilter definition to always apply auth to both http and https traffic, regardless of which is currently enabled in the Gateway. This is to prevent the possibility of these objects going out of sync (eg: Gateway allowing traffic on one port, and EnvoyFilter applying Auth on traffic through the other port), which could have occurred if the charm succeeded in updating one of those objects but failed to update the other (due to an error or incompatible settings). By always applying the EnvoyFilter to all traffic, this possibility is prevented.

This PR came from an attempt to address canonical/bundle-kubeflow#570, which showed our previously was misconfiguring the EnvoyFilter when using SSL. It was reported that we incorrectly applied the EnvoyFilter only to http traffic and not https traffic. Reproducing canonical/bundle-kubeflow#570 was a challenge, partly because the charm has gone through recent rewrites that might have fixed the issue. The specific cause of canonical/bundle-kubeflow#570 could not be confirmed. It may be fixed by this PR, but that is unclear. More discussion on this is in this thread.

Closes canonical/bundle-kubeflow#570

Testing instructions

# pack the modified charm (from this PR)
cd charms/istio-pilot
charmcraft pack

# deploy istio, dex/oidc, and the dashboard to have something to browse to.  
# For the initial deployment, do this without certs/using http
juju deploy ./istio-pilot_ubuntu-20.04-amd64.charm --trust
juju deploy istio-gateway --channel=1.15/stable --trust --config kind=ingress istio-ingressgateway
juju relate istio-pilot istio-ingressgateway

juju deploy dex-auth --channel=2.31/stable --trust --config static-username=user --config static-password=user --config public-url=http://10.64.140.43.nip.io
juju deploy oidc-gatekeeper --channel ckf-1.7/stable --config public-url=http://10.64.140.43.nip.io
juju relate istio-pilot:ingress dex-auth:ingress
juju relate dex-auth:oidc-client oidc-gatekeeper:oidc-client
juju relate istio-pilot:ingress oidc-gatekeeper:ingress
juju relate istio-pilot:ingress-auth oidc-gatekeeper:ingress-auth

juju deploy kubeflow-profiles --channel=1.7/stable --trust
juju deploy kubeflow-dashboard --channel=1.7/stable --trust
juju relate kubeflow-profiles kubeflow-dashboard
juju relate kubeflow-dashboard:ingress istio-pilot:ingress

# Check whether everything works as expected
# try to connect to 
# * http://10.64.140.43.nip.io/: redirected to dex login 
# * http://10.64.140.43.nip.io/dex/.well-known/openid-configuration: works
# * https://10.64.140.43.nip.io/: cannot be reached
# * https://10.64.140.43.nip.io/dex/.well-known/openid-configuration: cannot be reached
# http all works, https all does not, as expected

# Set up SSL by creating a self-signed cert.  Note:
# * if you used a real cert, you wouldn't have to update the oidc-gatekeeper ca-bundle. 
# * you must have a common name and SAN in the self-signed cert

openssl req -nodes -new -x509 \
 -subj "/CN=10.64.140.43.nip.io" \
 -addext "subjectAltName = DNS:10.64.140.43.nip.io" \
 -keyout server-san.key -out server-san.cert 

juju config istio-pilot ssl-crt="$(cat server-san.cert | base64 -w0)"
juju config istio-pilot ssl-key="$(cat server-san.key | base64 -w0)" 

juju config dex-auth public-url=https://10.64.140.43.nip.io

# Because oidc for 1.7 does not restart its workload if you change the ca-bundle, we remove and redeploy it
juju remove-application oidc-gatekeeper
juju deploy oidc-gatekeeper --channel ckf-1.7/stable --config public-url=https://10.64.140.43.nip.io --config ca-bundle="$(cat server-san.cert)"

# Re-relate oidc
juju relate dex-auth:oidc-client oidc-gatekeeper:oidc-client
juju relate istio-pilot:ingress oidc-gatekeeper:ingress
juju relate istio-pilot:ingress-auth oidc-gatekeeper:ingress-auth

# wait for oidc/dex-auth to do their thing...

# Check whether everything works.  Now, we should only be able to reach http urls, not https
# * http://10.64.140.43.nip.io/: cannot be reached
# * http://10.64.140.43.nip.io/dex/.well-known/openid-configuration: cannot be reached
# * https://10.64.140.43.nip.io: warning about insecure cert, then redirected to dex login
# * https://10.64.140.43.nip.io/dex/.well-known/openid-configuration: warning about insecure cert, then it works

@ca-scribner ca-scribner marked this pull request as ready for review May 29, 2023 19:35
@ca-scribner ca-scribner requested a review from a team as a code owner May 29, 2023 19:35
@ca-scribner ca-scribner marked this pull request as draft May 29, 2023 19:35
@ca-scribner ca-scribner marked this pull request as ready for review May 30, 2023 18:24
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @ca-scribner ! Some comments.

@i-chvets
Copy link
Contributor

i-chvets commented Jun 1, 2023

My only comment would be to move testing insructions into related issue instead of PR.

@ca-scribner
Copy link
Contributor Author

ca-scribner commented Jun 1, 2023

Good call @i-chvets - I'll make an issue, add the steps, and close the issue.

edit: done -> #280

@DnPlas
Copy link
Contributor

DnPlas commented Jun 5, 2023

Thanks @ca-scribner, LGTM

@ca-scribner ca-scribner merged commit 84ec149 into main Jun 5, 2023
@ca-scribner ca-scribner deleted the KF-1675-fix-ssl-authentication branch June 5, 2023 13:55
ca-scribner added a commit that referenced this pull request Jun 6, 2023
* always apply envoyfilter to both http and https traffic
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.

Configuring Charmed Kubeflow 1.7 with SSL removes all authentication
4 participants