From a4a319a76e3e65bc4770cf769c877656239515de Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 25 Nov 2022 10:10:09 +0000 Subject: [PATCH] feat: add CODER_OIDC_IGNORE_EMAIL_VERIFIED config knob (#5165) * Adds a configuration knob CODER_OIDC_IGNORE_EMAIL_VERIFIED that allows ignoring the email_verified OIDC claim * Adds warning message at startup if CODER_OIDC_IGNORE_EMAIL_VERIFIED=true * Adds warning whenever an unverified OIDC email is let through * Skips flaky test on non-linux platforms Co-authored-by: Mathias Fredriksson --- cli/deployment/config.go | 6 +++++ cli/deployment/config_test.go | 28 +++++++++++++++----- cli/server.go | 4 +++ cli/testdata/coder_server_--help.golden | 3 +++ coderd/userauth.go | 14 +++++++--- coderd/userauth_test.go | 34 ++++++++++++++++++++----- codersdk/deploymentconfig.go | 13 +++++----- docs/admin/auth.md | 10 ++++++++ loadtest/reconnectingpty/run_test.go | 4 +-- site/src/api/typesGenerated.ts | 1 + 10 files changed, 91 insertions(+), 26 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index 127b14df6f..9e1c921631 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -231,6 +231,12 @@ func newConfig() *codersdk.DeploymentConfig { Flag: "oidc-scopes", Default: []string{oidc.ScopeOpenID, "profile", "email"}, }, + IgnoreEmailVerified: &codersdk.DeploymentConfigField[bool]{ + Name: "OIDC Ignore Email Verified", + Usage: "Ignore the email_verified claim from the upstream provider.", + Flag: "oidc-ignore-email-verified", + Default: false, + }, }, Telemetry: &codersdk.TelemetryConfig{ diff --git a/cli/deployment/config_test.go b/cli/deployment/config_test.go index 515851439e..0d50455023 100644 --- a/cli/deployment/config_test.go +++ b/cli/deployment/config_test.go @@ -122,23 +122,37 @@ func TestConfig(t *testing.T) { require.Equal(t, config.Trace.Enable.Value, true) require.Equal(t, config.Trace.HoneycombAPIKey.Value, "my-honeycomb-key") }, + }, { + Name: "OIDC_Defaults", + Env: map[string]string{}, + Valid: func(config *codersdk.DeploymentConfig) { + require.Empty(t, config.OIDC.IssuerURL.Value) + require.Empty(t, config.OIDC.EmailDomain.Value) + require.Empty(t, config.OIDC.ClientID.Value) + require.Empty(t, config.OIDC.ClientSecret.Value) + require.True(t, config.OIDC.AllowSignups.Value) + require.ElementsMatch(t, config.OIDC.Scopes.Value, []string{"openid", "email", "profile"}) + require.False(t, config.OIDC.IgnoreEmailVerified.Value) + }, }, { Name: "OIDC", Env: map[string]string{ - "CODER_OIDC_ISSUER_URL": "https://accounts.google.com", - "CODER_OIDC_EMAIL_DOMAIN": "coder.com", - "CODER_OIDC_CLIENT_ID": "client", - "CODER_OIDC_CLIENT_SECRET": "secret", - "CODER_OIDC_ALLOW_SIGNUPS": "false", - "CODER_OIDC_SCOPES": "something,here", + "CODER_OIDC_ISSUER_URL": "https://accounts.google.com", + "CODER_OIDC_EMAIL_DOMAIN": "coder.com", + "CODER_OIDC_CLIENT_ID": "client", + "CODER_OIDC_CLIENT_SECRET": "secret", + "CODER_OIDC_ALLOW_SIGNUPS": "false", + "CODER_OIDC_SCOPES": "something,here", + "CODER_OIDC_IGNORE_EMAIL_VERIFIED": "true", }, Valid: func(config *codersdk.DeploymentConfig) { require.Equal(t, config.OIDC.IssuerURL.Value, "https://accounts.google.com") require.Equal(t, config.OIDC.EmailDomain.Value, "coder.com") require.Equal(t, config.OIDC.ClientID.Value, "client") require.Equal(t, config.OIDC.ClientSecret.Value, "secret") - require.Equal(t, config.OIDC.AllowSignups.Value, false) + require.False(t, config.OIDC.AllowSignups.Value) require.Equal(t, config.OIDC.Scopes.Value, []string{"something", "here"}) + require.True(t, config.OIDC.IgnoreEmailVerified.Value) }, }, { Name: "GitHub", diff --git a/cli/server.go b/cli/server.go index e486837f5b..8d304a14e9 100644 --- a/cli/server.go +++ b/cli/server.go @@ -398,6 +398,10 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co return xerrors.Errorf("configure oidc client certificates: %w", err) } + if cfg.OIDC.IgnoreEmailVerified.Value { + logger.Warn(ctx, "coder will not check email_verified for OIDC logins") + } + oidcProvider, err := oidc.NewProvider(ctx, cfg.OIDC.IssuerURL.Value) if err != nil { return xerrors.Errorf("configure oidc provider: %w", err) diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index f609657cc5..1351da7c89 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -98,6 +98,9 @@ Flags: --oidc-email-domain string Email domain that clients logging in with OIDC must match. Consumes $CODER_OIDC_EMAIL_DOMAIN + --oidc-ignore-email-verified Ignore the email_verified claim from the + upstream provider. + Consumes $CODER_OIDC_IGNORE_EMAIL_VERIFIED --oidc-issuer-url string Issuer URL to use for Login with OIDC. Consumes $CODER_OIDC_ISSUER_URL --oidc-scopes strings Scopes to grant when authenticating with diff --git a/coderd/userauth.go b/coderd/userauth.go index deffdca9c3..add41cf291 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -195,6 +195,9 @@ type OIDCConfig struct { // EmailDomain is the domain to enforce when a user authenticates. EmailDomain string AllowSignups bool + // IgnoreEmailVerified allows ignoring the email_verified claim + // from an upstream OIDC provider. See #5065 for context. + IgnoreEmailVerified bool } func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { @@ -264,10 +267,13 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { if ok { verified, ok := verifiedRaw.(bool) if ok && !verified { - httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", email), - }) - return + if !api.OIDCConfig.IgnoreEmailVerified { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", email), + }) + return + } + api.Logger.Warn(ctx, "allowing unverified oidc email %q") } } // The username is a required property in Coder. We make a best-effort diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 04abffedaa..8727bedae3 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -479,13 +479,14 @@ func TestUserOIDC(t *testing.T) { t.Parallel() for _, tc := range []struct { - Name string - Claims jwt.MapClaims - AllowSignups bool - EmailDomain string - Username string - AvatarURL string - StatusCode int + Name string + Claims jwt.MapClaims + AllowSignups bool + EmailDomain string + Username string + AvatarURL string + StatusCode int + IgnoreEmailVerified bool }{{ Name: "EmailOnly", Claims: jwt.MapClaims{ @@ -502,6 +503,24 @@ func TestUserOIDC(t *testing.T) { }, AllowSignups: true, StatusCode: http.StatusForbidden, + }, { + Name: "EmailNotAString", + Claims: jwt.MapClaims{ + "email": 3.14159, + "email_verified": false, + }, + AllowSignups: true, + StatusCode: http.StatusBadRequest, + }, { + Name: "EmailNotVerifiedIgnored", + Claims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": false, + }, + AllowSignups: true, + StatusCode: http.StatusTemporaryRedirect, + Username: "kyle", + IgnoreEmailVerified: true, }, { Name: "NotInRequiredEmailDomain", Claims: jwt.MapClaims{ @@ -593,6 +612,7 @@ func TestUserOIDC(t *testing.T) { config := conf.OIDCConfig() config.AllowSignups = tc.AllowSignups config.EmailDomain = tc.EmailDomain + config.IgnoreEmailVerified = tc.IgnoreEmailVerified client := coderdtest.New(t, &coderdtest.Options{ OIDCConfig: config, diff --git a/codersdk/deploymentconfig.go b/codersdk/deploymentconfig.go index 806cd59b76..ffe7ed4e57 100644 --- a/codersdk/deploymentconfig.go +++ b/codersdk/deploymentconfig.go @@ -87,12 +87,13 @@ type OAuth2GithubConfig struct { } type OIDCConfig struct { - AllowSignups *DeploymentConfigField[bool] `json:"allow_signups" typescript:",notnull"` - ClientID *DeploymentConfigField[string] `json:"client_id" typescript:",notnull"` - ClientSecret *DeploymentConfigField[string] `json:"client_secret" typescript:",notnull"` - EmailDomain *DeploymentConfigField[string] `json:"email_domain" typescript:",notnull"` - IssuerURL *DeploymentConfigField[string] `json:"issuer_url" typescript:",notnull"` - Scopes *DeploymentConfigField[[]string] `json:"scopes" typescript:",notnull"` + AllowSignups *DeploymentConfigField[bool] `json:"allow_signups" typescript:",notnull"` + ClientID *DeploymentConfigField[string] `json:"client_id" typescript:",notnull"` + ClientSecret *DeploymentConfigField[string] `json:"client_secret" typescript:",notnull"` + EmailDomain *DeploymentConfigField[string] `json:"email_domain" typescript:",notnull"` + IssuerURL *DeploymentConfigField[string] `json:"issuer_url" typescript:",notnull"` + Scopes *DeploymentConfigField[[]string] `json:"scopes" typescript:",notnull"` + IgnoreEmailVerified *DeploymentConfigField[bool] `json:"ignore_email_verified" typescript:",notnull"` } type TelemetryConfig struct { diff --git a/docs/admin/auth.md b/docs/admin/auth.md index 877ef1d8af..b65efaeeb6 100644 --- a/docs/admin/auth.md +++ b/docs/admin/auth.md @@ -76,11 +76,21 @@ 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 ``` +Coder requires all OIDC email addresses to be verified by default. If the `email_verified` claim is present in the token response from the identity provider, Coder will validate that its value is `true`. +If needed, you can disable this behavior with the following setting: + +```console +CODER_OIDC_IGNORE_EMAIL_VERIFIED=true +``` + +> **Note:** This will cause Coder to implicitly treat all OIDC emails as "verified". + ## SCIM (enterprise) Coder supports user provisioning and deprovisioning via SCIM 2.0 with header diff --git a/loadtest/reconnectingpty/run_test.go b/loadtest/reconnectingpty/run_test.go index dc68251141..f691fed514 100644 --- a/loadtest/reconnectingpty/run_test.go +++ b/loadtest/reconnectingpty/run_test.go @@ -23,8 +23,8 @@ import ( func Test_Runner(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { - t.Skip("PTY is flakey on Windows") + if runtime.GOOS != "linux" { + t.Skip("PTY is flakey on non-Linux platforms") } t.Run("OK", func(t *testing.T) { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 2347d8f680..ed54708059 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -441,6 +441,7 @@ export interface OIDCConfig { readonly email_domain: DeploymentConfigField readonly issuer_url: DeploymentConfigField readonly scopes: DeploymentConfigField + readonly ignore_email_verified: DeploymentConfigField } // From codersdk/organizations.go