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: propagate http.Client to JWT verifier for OIDC connector #3641

Merged
merged 1 commit into from
Jul 24, 2024
Merged

fix: propagate http.Client to JWT verifier for OIDC connector #3641

merged 1 commit into from
Jul 24, 2024

Conversation

jack-r-warren
Copy link
Contributor

@jack-r-warren jack-r-warren commented Jul 22, 2024

Overview

The OIDC connector has two options that directly customize the http.Client used for requests: rootCAs and insecureSkipVerify (passed here to http.Client here).

That customized http.Client isn't currently propagated through to the JWT verifier. This means that the verifier's requests to the JWKS endpoint can unexpectedly fail with certificate issues.

Example of an error caused by this inconsistency

Truncated sample configuration describing our setup:

connectors:
  - type: oidc
    # omitting other config
    issuer: https://<in-cluster-hostname>.svc.cluster.local
    insecureSkipVerify: true
    providerDiscoveryOverrides:
      authURL: https://<public-hostname-behind-auth-proxy>.org

Dex's communication with the provider should be inside the Kubernetes cluster so Dex doesn't need to worry about the authenticating proxy in front of the public URLs. Dex logs the following error upon attempted login:

Failed to authenticate: oidc: failed to verify ID Token: failed to verify signature: fetching keys oidc: get keys failed Get \"https://<in-cluster-hostname>.svc.cluster.local/keys\": tls: failed to verify certificate: x509: certificate is valid for <public-hostname-behind-auth-proxy>.org, not <in-cluster-hostname>.svc.cluster.local

Even if we were to self-sign certificates for in-cluster hostnames, we wouldn't be able to use the rootCAs option to make the verifier respect that either.

What this PR does / why we need it

  • Call coreos/go-oidc's Provider.VerifierContext instead of Provider.Verifier, so we can pass the same ctx-with-customized-http.Client that's used elsewhere.
    • The docs for those functions specifically say to use VerifierContext to configure how requests are made.
    • The call chain for those interested is 1, 2, 3, 4, 5, 6, and the client is set in the ctx here.

Special notes for your reviewer

This PR closely follows prior art from #2781.

It doesn't appear that there's a great way to test this sort of TLS behavior here; #2781 didn't include tests either. We're currently running this patch in production and it's solved the issue. I suppose it'd be possible to reach into the JWT verifier with reflection to check that the ctx has the expected value set... but that seems brittle to me. Happy for suggestions.

@nabokihms nabokihms added the release-note/enhancement Release note: Enhancements label Jul 24, 2024
@nabokihms
Copy link
Member

Good catch, thank you, Jack!

@nabokihms nabokihms merged commit 849d601 into dexidp:master Jul 24, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/enhancement Release note: Enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants