Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
105 changes: 82 additions & 23 deletions cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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!")
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
167 changes: 167 additions & 0 deletions cli/server_fix_oidc_links_internal_test.go
Original file line number Diff line number Diff line change
@@ -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")
})
}
6 changes: 6 additions & 0 deletions cli/testdata/server-config.yaml.golden
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,12 @@ oidc:
# setting can also break OIDC, so use with caution.
# (default: <unset>, 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.
Expand Down
3 changes: 3 additions & 0 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions coderd/apidoc/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions codersdk/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,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 {
Expand Down Expand Up @@ -2994,6 +2996,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,
{
Expand Down
1 change: 1 addition & 0 deletions docs/reference/api/general.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading