-
Notifications
You must be signed in to change notification settings - Fork 905
fix: Add client certs to OAuth HTTPClient context #5126
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: Add client certs to OAuth HTTPClient context #5126
Conversation
coderd/httpmw/oauth2.go
Outdated
}) | ||
return | ||
} | ||
|
||
oauthToken, err := config.Exchange(ctx, code) |
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.
This line was where my Coder was crashing out on auth
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
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.
Thanks for reporting this issue and making an attempt at a PR! I think we should avoid moving logic to codersdk
(or even outside cli/server
). If you want to make an attempt at rewriting this so the http.Client
is still initialized in cli/server.go
, but made usable by Exchange
, that would be much appreciated.
(Sorry for the spam) Ok ok ok I think I've got it working in a much cleaner way 👍 Lmk what you think |
Had the time this weekend to setup my OIDC provider to mimic what we have at work (require TLS client certs) and can confirm it now works Will deploy out on Monday to verify it works in that 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.
Thanks! Excellent solution, it's very close to what I was imagining. We might iterate on this in the future to make sure the client is used in all requests performed by the server 👍🏻.
Howdy all,
I deployed out Coder 12.7 with my client cert changes (#5042) and got past the
.well-known
check that the OIDC library makesBut then Coder crashed out when I tried to authenticate due to missing client certs when hitting my OIDC providers' Auth endpoint
I pulled out some common methods into
codersdk/certificateUtils.go
to avoid a cyclical dependency. Let me know if that should live in a different placeThanks!