Skip to content

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 13, 2022
Merged

OAuth now uses client TLS certs (if configured) #5042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0490733
OAuth now uses client TLS certs (if configured)
normana10 Nov 12, 2022
54ee429
Update docs
normana10 Nov 12, 2022
ae0c5dd
Cleaning
normana10 Nov 12, 2022
5df2ca8
Fix lint errors and generate static files
normana10 Nov 13, 2022
d71be77
Fix lint error and regenerate more static files
normana10 Nov 13, 2022
83fcf35
Suppress lint error
normana10 Nov 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cli/deployment/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,16 @@ func newConfig() *codersdk.DeploymentConfig {
Flag: "tls-min-version",
Default: "tls12",
},
ClientCertFile: &codersdk.DeploymentConfigField[string]{
Name: "TLS Client Cert File",
Usage: "Path to certificate for client TLS authentication. It requires a PEM-encoded file.",
Flag: "tls-client-cert-file",
},
ClientKeyFile: &codersdk.DeploymentConfigField[string]{
Name: "TLS Client Key File",
Usage: "Path to key for client TLS authentication. It requires a PEM-encoded file.",
Flag: "tls-client-key-file",
},
},
Trace: &codersdk.TraceConfig{
Enable: &codersdk.DeploymentConfigField[bool]{
Expand Down
23 changes: 23 additions & 0 deletions cli/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 != "" {
Copy link
Member

Choose a reason for hiding this comment

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

We already have ClientCAFile as a configuration option, so you should be able to remove the configuration changes here!

Copy link
Contributor Author

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 I think it's slightly different

So with my changes in the config file we have:

  • CertFiles
  • KeyFiles
  • ClientCAFile
  • ClientCertFile (new)
  • ClientKeyFile (new)

In my org, I have been given a wildcard cert *.coder.normana10.example.com and a "system/identity" cert normana10.example.com

I need to use the wildcard cert for all "serving" and the "identity" cert for all external calls that 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.cert
  • KeyFiles = /path/to/wildcard.key
  • ClientCAFile = /path/to/ca.cert (Only used to verify client certs if ClientAuth is set to verify)
  • ClientCertFile (new) = /path/to/identity.cert
  • ClientKeyFile (new) = /path/to/identity.key

So ClientCAFile (as far as I've seen) looks like it's just used to verify clients connecting to Coder. Where I'm looking to set the client certs Coder uses when connecting to external services

If that makes sense?

For full transparency, my actual config doesn't have Coder terminating TLS, so realistically I'd just have my two new configs set

Copy link
Member

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!

Copy link
Member

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!

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
}
8 changes: 8 additions & 0 deletions cli/testdata/coder_server_--help.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,14 @@ Flags:
used for checking the authenticity of
client
Consumes $CODER_TLS_CLIENT_CA_FILE
--tls-client-cert-file string Path to certificate for client TLS
authentication. It requires a PEM-encoded
file.
Consumes $CODER_TLS_CLIENT_CERT_FILE
--tls-client-key-file string Path to key for client TLS
authentication. It requires a PEM-encoded
file.
Consumes $CODER_TLS_CLIENT_KEY_FILE
--tls-enable Whether TLS will be enabled.
Consumes $CODER_TLS_ENABLE
--tls-key-file strings Paths to the private keys for each of the
Expand Down
14 changes: 8 additions & 6 deletions codersdk/deploymentconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,14 @@ type TelemetryConfig struct {
}

type TLSConfig struct {
Enable *DeploymentConfigField[bool] `json:"enable" typescript:",notnull"`
CertFiles *DeploymentConfigField[[]string] `json:"cert_file" typescript:",notnull"`
ClientAuth *DeploymentConfigField[string] `json:"client_auth" typescript:",notnull"`
ClientCAFile *DeploymentConfigField[string] `json:"client_ca_file" typescript:",notnull"`
KeyFiles *DeploymentConfigField[[]string] `json:"key_file" typescript:",notnull"`
MinVersion *DeploymentConfigField[string] `json:"min_version" typescript:",notnull"`
Enable *DeploymentConfigField[bool] `json:"enable" typescript:",notnull"`
CertFiles *DeploymentConfigField[[]string] `json:"cert_file" typescript:",notnull"`
ClientAuth *DeploymentConfigField[string] `json:"client_auth" typescript:",notnull"`
ClientCAFile *DeploymentConfigField[string] `json:"client_ca_file" typescript:",notnull"`
KeyFiles *DeploymentConfigField[[]string] `json:"key_file" typescript:",notnull"`
MinVersion *DeploymentConfigField[string] `json:"min_version" typescript:",notnull"`
ClientCertFile *DeploymentConfigField[string] `json:"client_cert_file" typescript:",notnull"`
ClientKeyFile *DeploymentConfigField[string] `json:"client_key_file" typescript:",notnull"`
}

type TraceConfig struct {
Expand Down
6 changes: 6 additions & 0 deletions docs/admin/auth.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ Once complete, run `sudo service coder restart` to reboot Coder.

> When a new user is created, the `preferred_username` claim becomes the username. If this claim is empty, the email address will be stripped of the domain, and become the username (e.g. `example@coder.com` becomes `example`).

If your OpenID Connect provider requires client TLS certificates for authentication, you can configure them like so:
```console
CODER_TLS_CLIENT_CERT_FILE=/path/to/cert.pem
CODER_TLS_CLIENT_KEY_FILE=/path/to/key.pem
```

## SCIM (enterprise)

Coder supports user provisioning and deprovisioning via SCIM 2.0 with header
Expand Down
2 changes: 1 addition & 1 deletion scripts/apitypings/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ type TypescriptTypes struct {
// String just combines all the codeblocks.
func (t TypescriptTypes) String() string {
var s strings.Builder
_, _ = s.WriteString("// Code generated by 'make coder/scripts/apitypings/main.go'. DO NOT EDIT.\n\n")
_, _ = s.WriteString("// Code generated by 'make site/src/api/typesGenerated.ts'. DO NOT EDIT.\n\n")

sortedTypes := make([]string, 0, len(t.Types))
sortedEnums := make([]string, 0, len(t.Enums))
Expand Down
2 changes: 1 addition & 1 deletion scripts/apitypings/testdata/enums/enums.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Code generated by 'make coder/scripts/apitypings/main.go'. DO NOT EDIT.
// Code generated by 'make site/src/api/typesGenerated.ts'. DO NOT EDIT.

// From codersdk/enums.go
export type Enum = "bar" | "baz" | "foo" | "qux"
2 changes: 1 addition & 1 deletion scripts/apitypings/testdata/generics/generics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Code generated by 'make coder/scripts/apitypings/main.go'. DO NOT EDIT.
// Code generated by 'make site/src/api/typesGenerated.ts'. DO NOT EDIT.

// From codersdk/generics.go
export interface ComplexGeneric<C extends comparable, S extends Single, T extends Custom> {
Expand Down
4 changes: 3 additions & 1 deletion site/src/api/typesGenerated.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Code generated by 'make coder/scripts/apitypings/main.go'. DO NOT EDIT.
// Code generated by 'make site/src/api/typesGenerated.ts'. DO NOT EDIT.

// From codersdk/apikey.go
export interface APIKey {
Expand Down Expand Up @@ -598,6 +598,8 @@ export interface TLSConfig {
readonly client_ca_file: DeploymentConfigField<string>
readonly key_file: DeploymentConfigField<string[]>
readonly min_version: DeploymentConfigField<string>
readonly client_cert_file: DeploymentConfigField<string>
readonly client_key_file: DeploymentConfigField<string>
}

// From codersdk/deploymentconfig.go
Expand Down