- 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
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
049073354ee429ae0c5dd5df2ca8d71be7783fcf35File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -392,6 +392,11 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co | ||
| return xerrors.Errorf("OIDC issuer URL must be set!") | ||
| } | ||
| ctx, err := handleOauth2ClientCertificates(ctx, cfg) | ||
| if err != nil { | ||
| return xerrors.Errorf("configure oidc client certificates: %w", err) | ||
| } | ||
| oidcProvider, err := oidc.NewProvider(ctx, cfg.OIDC.IssuerURL.Value) | ||
| if err != nil { | ||
| return xerrors.Errorf("configure oidc provider: %w", err) | ||
| @@ -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 commentThe reason will be displayed to describe this comment to others.Learn more. We already have There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
In my org, I have been given a wildcard cert 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:
So 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 commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! | ||
| certificates, err := loadCertificates([]string{cfg.TLS.ClientCertFile.Value}, []string{cfg.TLS.ClientKeyFile.Value}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return context.WithValue(ctx, oauth2.HTTPClient, &http.Client{ | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ //nolint:gosec | ||
| Certificates: certificates, | ||
| }, | ||
| }, | ||
| }), nil | ||
| } | ||
| return ctx, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| // Code generated by 'makesite/src/api/typesGenerated.ts'. DO NOT EDIT. | ||
| // From codersdk/enums.go | ||
| export type Enum = "bar" | "baz" | "foo" | "qux" |