- Notifications
You must be signed in to change notification settings - Fork928
OAuth now uses client TLS certs (if configured)#5042
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
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.
Your contribution is appreciated!
Is it possible for us to use the existingClientCAFile property instead of adding these new ones? RIght now, it's consumed when TLS is enabled, but it seems like we should make italways apply instead and add theoauth2.HTTPClient to the context.
If you need help or have questions, feel free to ping me! Happy to add this (and we can do a release afterward so you can use it).
Uh oh!
There was an error while loading.Please reload this page.
| @@ -1248,3 +1253,21 @@ func startBuiltinPostgres(ctx context.Context, cfg config.Root, logger slog.Logg | |||
| } | |||
| return connectionURL, ep.Stop, nil | |||
| } | |||
| func handleOauth2ClientCertificates(cfg *codersdk.DeploymentConfig, ctx context.Context) (context.Context, error) { | |||
| if cfg.TLS.ClientCertFile.Value != "" && cfg.TLS.ClientKeyFile.Value != "" { | |||
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.
We already haveClientCAFile as a configuration option, so you should be able to remove the configuration changes here!
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'm going to expand on my use-case a bit more because Ithink it's slightly different
So with my changes in the config file we have:
CertFilesKeyFilesClientCAFileClientCertFile(new)ClientKeyFile(new)
In my org, I have been given a wildcard cert*.coder.normana10.example.com and a "system/identity" certnormana10.example.com
I need to use the wildcard cert for all "serving" and the "identity" cert for all external callsthat Coder makes
(I'm not going to say this is "correct", but it's just how my org does things)
So if I wanted Coder to terminate TLS my config would look like:
CertFiles=/path/to/wildcard.certKeyFiles=/path/to/wildcard.keyClientCAFile=/path/to/ca.cert(Only used to verify client certs ifClientAuthis set to verify)ClientCertFile(new) =/path/to/identity.certClientKeyFile(new) =/path/to/identity.key
SoClientCAFile (as far as I've seen) looks like it's just used to verifyclients connecting to Coder. Where I'm looking to set the client certsCoder uses when connecting to external services
If that makes sense?
For full transparency, myactual config doesn't have Coder terminating TLS, so realistically I'djust have my two new configs set
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.
All makes sense! I appreciate the context... the changes look good to me!
| @@ -1248,3 +1253,21 @@ func startBuiltinPostgres(ctx context.Context, cfg config.Root, logger slog.Logg | |||
| } | |||
| return connectionURL, ep.Stop, nil | |||
| } | |||
| func handleOauth2ClientCertificates(ctx context.Context, cfg *codersdk.DeploymentConfig) (context.Context, error) { | |||
| if cfg.TLS.ClientCertFile.Value != "" && cfg.TLS.ClientKeyFile.Value != "" { | |||
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.
nit (not blocking merge): You could invert the control-flow here to reduce indentation!
Hi,
I'm trying to get Coder deployed within my organization but our OpenID Connect providerrequires valid TLS client certificates before it will respond to the
.well-knownHTTP call that theoidclibrary makesThese changes configure the
oidclibrary to use (optional and configurable) client TLS certificatesI've verified these changes work withmy personal/homelab OpenID Connect provider when I configure my reverse proxy to require TLS client certs
(Let me know if I missed anything, I'm new to Go 😄)
Also let me know if there's some glaringly obvious Go-ish way to do this with like environment variables or command line args that I'm unaware of
Thanks!