diff --git a/cli/server.go b/cli/server.go index f61e561cff..7210ade859 100644 --- a/cli/server.go +++ b/cli/server.go @@ -65,6 +65,7 @@ import ( "github.com/coder/coder/v2/cli/config" "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/aibridged" + "github.com/coder/coder/v2/coderd/authlink" "github.com/coder/coder/v2/coderd/autobuild" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/awsiamrds" @@ -117,6 +118,58 @@ import ( "github.com/coder/wgtunnel/tunnelsdk" ) +// oidcAuthLinks validates and can repair any broken OIDC auth links from changes in +// OIDC providers. This function should avoid returning a fatal error as much as possible. +// If this function fails, it should just log the error and exit. +func oidcAuthLinks(ctx context.Context, logger slog.Logger, cli *http.Client, vals *codersdk.DeploymentValues, db database.Store) error { + // nolint:gocritic // Requires system privileges + ctx = dbauthz.AsSystemRestricted(ctx) + expectedIssuer, err := authlink.ResolveIssuer(ctx, cli, vals.OIDC.IssuerURL.String()) + if err != nil { + // Always log if there is a failure here + logger.Error(ctx, "unable to resolve OIDC 'issuer'", + slog.F("error", err.Error()), + slog.F("url", vals.OIDC.IssuerURL.String()), + ) + return nil + } + + analysis, err := authlink.AnalyzeOIDCLinks(ctx, db, expectedIssuer) + if err != nil { + // Do not make this error fatal + logger.Error(ctx, "unable to analyze OIDC links, OIDC user links cannot be verified as linked to this issuer", + slog.F("error", err.Error()), + slog.F("url", vals.OIDC.IssuerURL.String()), + slog.F("issuer", expectedIssuer), + ) + return nil + } + + if !vals.OIDC.AutoRepairLinks.Value() { + return nil + } + + // Repair any broken OIDC links + if analysis.MismatchedTotal() > 0 { + count, err := authlink.ResetMismatchedOIDCLinks(ctx, db, expectedIssuer) + if err != nil { + logger.Error(ctx, "unable to reset mismatched OIDC links", + slog.F("error", err.Error()), + slog.F("url", vals.OIDC.IssuerURL.String()), + slog.F("issuer", expectedIssuer), + ) + return nil + } + + logger.Info(ctx, "oidc users OIDC links reset", + slog.F("url", vals.OIDC.IssuerURL.String()), + slog.F("issuer", expectedIssuer), + slog.F("count", count), + ) + } + return nil +} + func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) { if vals.OIDC.ClientID == "" { return nil, xerrors.Errorf("OIDC client ID must be set!") @@ -720,29 +773,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. } } - // As OIDC clients can be confidential or public, - // we should only check for a client id being set. - // The underlying library handles the case of no - // client secrets correctly. For more details on - // client types: https://oauth.net/2/client-types/ - if vals.OIDC.ClientID != "" { - if vals.OIDC.IgnoreEmailVerified { - logger.Warn(ctx, "coder will not check email_verified for OIDC logins") - } - - // This OIDC config is **not** being instrumented with the - // oauth2 instrument wrapper. If we implement the missing - // oidc methods, then we can instrument it. - // Missing: - // - Userinfo - // - Verify - oc, err := createOIDCConfig(ctx, options.Logger, vals) - if err != nil { - return xerrors.Errorf("create oidc config: %w", err) - } - options.OIDCConfig = oc - } - // We'll read from this channel in the select below that tracks shutdown. If it remains // nil, that case of the select will just never fire, but it's important not to have a // "bare" read on this channel. @@ -838,6 +868,35 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. return xerrors.Errorf("set deployment id: %w", err) } + // As OIDC clients can be confidential or public, + // we should only check for a client id being set. + // The underlying library handles the case of no + // client secrets correctly. For more details on + // client types: https://oauth.net/2/client-types/ + if vals.OIDC.ClientID != "" { + if vals.OIDC.IgnoreEmailVerified { + logger.Warn(ctx, "coder will not check email_verified for OIDC logins") + } + + // This OIDC config is **not** being instrumented with the + // oauth2 instrument wrapper. If we implement the missing + // oidc methods, then we can instrument it. + // Missing: + // - Userinfo + // - Verify + oc, err := createOIDCConfig(ctx, options.Logger, vals) + if err != nil { + return xerrors.Errorf("create oidc config: %w", err) + } + options.OIDCConfig = oc + + // Repair any existing broken OIDC + err = oidcAuthLinks(ctx, logger, httpClient, vals, options.Database) + if err != nil { + return xerrors.Errorf("oidc auth links: %w", err) + } + } + extAuthEnv, err := ReadExternalAuthProvidersFromEnv(os.Environ()) if err != nil { return xerrors.Errorf("read external auth providers from env: %w", err) diff --git a/cli/server_fix_oidc_links_internal_test.go b/cli/server_fix_oidc_links_internal_test.go new file mode 100644 index 0000000000..71ce3389c1 --- /dev/null +++ b/cli/server_fix_oidc_links_internal_test.go @@ -0,0 +1,167 @@ +package cli + +import ( + "database/sql" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "cdr.dev/slog/v3" + "cdr.dev/slog/v3/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" + "github.com/coder/serpent" +) + +func fakeOIDCIssuerDiscovery(t *testing.T, issuer string) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/.well-known/openid-configuration" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": issuer, + }) + })) + t.Cleanup(srv.Close) + return srv +} + +func newDeploymentValues(t *testing.T, issuerURL string, autoRepair bool) *codersdk.DeploymentValues { + t.Helper() + vals := &codersdk.DeploymentValues{} + require.NoError(t, vals.OIDC.IssuerURL.Set(issuerURL)) + vals.OIDC.AutoRepairLinks = serpent.Bool(autoRepair) + return vals +} + +func TestOIDCAuthLinks(t *testing.T) { + t.Parallel() + + t.Run("RepairsMismatchedLinks", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + logger := testutil.Logger(t) + + const expectedIssuer = "https://accounts.google.com" + oidcSrv := fakeOIDCIssuerDiscovery(t, expectedIssuer) + + db, _ := dbtestutil.NewDB(t) + + // Correctly linked user. + correctUser := dbgen.User(t, db, database.User{LoginType: database.LoginTypeOIDC}) + dbgen.UserLink(t, db, database.UserLink{ + UserID: correctUser.ID, + LoginType: database.LoginTypeOIDC, + LinkedID: expectedIssuer + "||sub-correct", + }) + + // Mismatched user. + mismatchedUser := dbgen.User(t, db, database.User{LoginType: database.LoginTypeOIDC}) + dbgen.UserLink(t, db, database.UserLink{ + UserID: mismatchedUser.ID, + LoginType: database.LoginTypeOIDC, + LinkedID: "https://old-issuer.example.com||sub-mismatched", + }) + + vals := newDeploymentValues(t, oidcSrv.URL, true) + err := oidcAuthLinks(ctx, logger, oidcSrv.Client(), vals, db) + require.NoError(t, err) + + // Mismatched link should be reset. + link, err := db.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{ + UserID: mismatchedUser.ID, + LoginType: database.LoginTypeOIDC, + }) + require.NoError(t, err) + require.Equal(t, "", link.LinkedID) + + // Correct link should be untouched. + link, err = db.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{ + UserID: correctUser.ID, + LoginType: database.LoginTypeOIDC, + }) + require.NoError(t, err) + require.Equal(t, expectedIssuer+"||sub-correct", link.LinkedID) + }) + + t.Run("AutoRepairDisabledSkipsReset", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + logger := testutil.Logger(t) + + const expectedIssuer = "https://accounts.google.com" + oidcSrv := fakeOIDCIssuerDiscovery(t, expectedIssuer) + + db, _ := dbtestutil.NewDB(t) + + mismatchedUser := dbgen.User(t, db, database.User{LoginType: database.LoginTypeOIDC}) + dbgen.UserLink(t, db, database.UserLink{ + UserID: mismatchedUser.ID, + LoginType: database.LoginTypeOIDC, + LinkedID: "https://old-issuer.example.com||sub-mismatched", + }) + + vals := newDeploymentValues(t, oidcSrv.URL, false) + err := oidcAuthLinks(ctx, logger, oidcSrv.Client(), vals, db) + require.NoError(t, err) + + // Link must not be modified when auto-repair is off. + link, err := db.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{ + UserID: mismatchedUser.ID, + LoginType: database.LoginTypeOIDC, + }) + require.NoError(t, err) + require.Equal(t, "https://old-issuer.example.com||sub-mismatched", link.LinkedID) + }) + + t.Run("DiscoveryFailureNonFatal", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + // oidcAuthLinks logs errors but does not return them. + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + // Serve 500 so discovery fails. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + t.Cleanup(srv.Close) + + db, _ := dbtestutil.NewDB(t) + + vals := newDeploymentValues(t, srv.URL, true) + err := oidcAuthLinks(ctx, logger, srv.Client(), vals, db) + require.NoError(t, err, "discovery failure must not be fatal") + }) + + t.Run("AnalyzeFailureNonFatal", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + // oidcAuthLinks logs errors but does not return them. + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + const expectedIssuer = "https://accounts.google.com" + oidcSrv := fakeOIDCIssuerDiscovery(t, expectedIssuer) + + // Use a closed DB so the analysis query fails. + connectionURL, err := dbtestutil.Open(t) + require.NoError(t, err) + sqlDB, err := sql.Open("postgres", connectionURL) + require.NoError(t, err) + db := database.New(sqlDB) + // Close the underlying connection so the query fails. + sqlDB.Close() + + vals := newDeploymentValues(t, oidcSrv.URL, true) + err = oidcAuthLinks(ctx, logger, oidcSrv.Client(), vals, db) + require.NoError(t, err, "analysis failure must not be fatal") + }) +} diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index 525f05d75f..80350aefb2 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -428,6 +428,12 @@ oidc: # setting can also break OIDC, so use with caution. # (default: , type: url) oidc-redirect-url: + # OIDC based users require the IdP issuer and subject in the claims to be static. + # If a new provider is configured, this option is required to be 'true'. It will + # reset any existing users to the previous provider, and match by email on their + # next login. + # (default: true, type: bool) + oidc-repair-links: true # Telemetry is critical to our ability to improve Coder. We strip all personal # information before sending data to our servers. Please only disable telemetry # when required by your organization's security policy. diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index fda0503008..66e8b9f231 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -21024,6 +21024,9 @@ const docTemplate = `{ "auth_url_params": { "type": "object" }, + "auto_repair_links": { + "type": "boolean" + }, "client_cert_file": { "type": "string" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index a0d99c05d3..8a6ebb0edf 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -19169,6 +19169,9 @@ "auth_url_params": { "type": "object" }, + "auto_repair_links": { + "type": "boolean" + }, "client_cert_file": { "type": "string" }, diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 5916ef7ab1..f1d91d2897 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -965,6 +965,8 @@ type OIDCConfig struct { // situations where the OIDC callback domain is different from the ACCESS_URL // domain. RedirectURL serpent.URL `json:"redirect_url" typescript:",notnull"` + + AutoRepairLinks serpent.Bool `json:"auto_repair_links" typescript:",notnull"` } type TelemetryConfig struct { @@ -3003,6 +3005,23 @@ func (c *DeploymentValues) Options() serpent.OptionSet { // So hide it, and only surface it to the small number of users that need it. Hidden: true, }, + { + Name: "OIDC Auto Repair Links", + Description: "OIDC based users require the IdP issuer and subject in the claims to be static. " + + "If a new provider is configured, this option is required to be 'true'. It will reset any existing users to the " + + "previous provider, and match by email on their next login.", + Required: false, + Default: "true", + Flag: "oidc-repair-links", + Env: "CODER_OIDC_REPAIR_LINKS", + YAML: "oidc-repair-links", + Value: &c.OIDC.AutoRepairLinks, + Group: &deploymentGroupOIDC, + UseInstead: nil, + // This flag should be removed after validation in real deployments. Leaving it + // as a flag as an escape hatch for now. + Hidden: true, + }, // Telemetry settings telemetryEnable, { diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index 71ca9ed014..4ae0f8d73b 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -424,6 +424,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "oidc": { "allow_signups": true, "auth_url_params": {}, + "auto_repair_links": true, "client_cert_file": "string", "client_id": "string", "client_key_file": "string", diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 57d997170f..d691e36f5f 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -5727,6 +5727,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "oidc": { "allow_signups": true, "auth_url_params": {}, + "auto_repair_links": true, "client_cert_file": "string", "client_id": "string", "client_key_file": "string", @@ -6328,6 +6329,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "oidc": { "allow_signups": true, "auth_url_params": {}, + "auto_repair_links": true, "client_cert_file": "string", "client_id": "string", "client_key_file": "string", @@ -8808,6 +8810,7 @@ Only certain features set these fields: - FeatureManagedAgentLimit| { "allow_signups": true, "auth_url_params": {}, + "auto_repair_links": true, "client_cert_file": "string", "client_id": "string", "client_key_file": "string", @@ -8878,6 +8881,7 @@ Only certain features set these fields: - FeatureManagedAgentLimit| |--------------------------------------|----------------------------------|----------|--------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `allow_signups` | boolean | false | | | | `auth_url_params` | object | false | | | +| `auto_repair_links` | boolean | false | | | | `client_cert_file` | string | false | | | | `client_id` | string | false | | | | `client_key_file` | string | false | | Client key file & ClientCertFile are used in place of ClientSecret for PKI auth. | diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index ce57f2281e..a5ced43139 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -6078,6 +6078,7 @@ export interface OIDCConfig { * domain. */ readonly redirect_url: string; + readonly auto_repair_links: boolean; } // From codersdk/parameters.go