coder · Emyrk · Jul 24, 2024 · Jul 23, 2024 · Jul 23, 2024 · Jul 23, 2024
diff --git a/cli/server.go b/cli/server.go
@@ -106,14 +106,20 @@ import (
 	"github.com/coder/coder/v2/tailnet"
 )
 
-func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) {
+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!")
 	}
 	if vals.OIDC.IssuerURL == "" {
 		return nil, xerrors.Errorf("OIDC issuer URL must be set!")
 	}
 
+	// Skipping issuer checks is not recommended.
+	if vals.OIDC.SkipIssuerChecks {
+		logger.Warn(ctx, "issuer checks with OIDC is disabled. This is not recommended as it can compromise the security of the authentication")
+		ctx = oidc.InsecureIssuerURLContext(ctx, vals.OIDC.IssuerURL.String())
+	}
+
 	oidcProvider, err := oidc.NewProvider(
 		ctx, vals.OIDC.IssuerURL.String(),
 	)
@@ -167,6 +173,9 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co
 		Provider:     oidcProvider,
 		Verifier: oidcProvider.Verifier(&oidc.Config{
 			ClientID: vals.OIDC.ClientID.String(),
+			// Enabling this skips checking the "iss" claim in the token
+			// matches the issuer URL. This is not recommended.
+			SkipIssuerCheck: vals.OIDC.SkipIssuerChecks.Value(),
 		}),
 		EmailDomain:         vals.OIDC.EmailDomain,
 		AllowSignups:        vals.OIDC.AllowSignups.Value(),
@@ -657,7 +666,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
 				// Missing:
 				//	- Userinfo
 				//	- Verify
-				oc, err := createOIDCConfig(ctx, vals)
+				oc, err := createOIDCConfig(ctx, options.Logger, vals)
 				if err != nil {
 					return xerrors.Errorf("create oidc config: %w", err)
 				}

diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden
@@ -513,6 +513,12 @@ OIDC OPTIONS:
           The custom text to show on the error page informing about disabled
           OIDC signups. Markdown format is supported.
 
+      --dangerous-oidc-skip-issuer-checks bool, $CODER_DANGEROUS_OIDC_SKIP_ISSUER_CHECKS
+          OIDC issuer urls must match in the request, the id_token 'iss' claim,
+          and in the well-known configuration. This flag disables that
+          requirement, and can lead to an insecure OIDC configuration. It is not
+          recommended to use this flag.
+
 PROVISIONING OPTIONS: 
 Tune the behavior of the provisioner, which is responsible for creating,
 updating, and deleting workspace resources.

diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden
@@ -364,6 +364,11 @@ oidc:
   # Markdown format is supported.
   # (default: <unset>, type: string)
   signupsDisabledText: ""
+  # OIDC issuer urls must match in the request, the id_token 'iss' claim, and in the
+  # well-known configuration. This flag disables that requirement, and can lead to
+  # an insecure OIDC configuration. It is not recommended to use this flag.
+  # (default: <unset>, type: bool)
+  dangerousSkipIssuerChecks: false
 # 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
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go
@@ -97,6 +97,9 @@ type FakeIDP struct {
 	deviceCode *syncmap.Map[string, deviceFlow]
 
 	// hooks
+	// hookWellKnown allows mutating the returned .well-known/configuration JSON.
+	// Using this can break the IDP configuration, so be careful.
+	hookWellKnown func(r *http.Request, j *ProviderJSON) error
 	// hookValidRedirectURL can be used to reject a redirect url from the
 	// IDP -> Application. Almost all IDPs have the concept of
 	// "Authorized Redirect URLs". This can be used to emulate that.
@@ -151,6 +154,12 @@ func WithMiddlewares(mws ...func(http.Handler) http.Handler) func(*FakeIDP) {
 	}
 }
 
+func WithHookWellKnown(hook func(r *http.Request, j *ProviderJSON) error) func(*FakeIDP) {
+	return func(f *FakeIDP) {
+		f.hookWellKnown = hook
+	}
+}
+
 // WithRefresh is called when a refresh token is used. The email is
 // the email of the user that is being refreshed assuming the claims are correct.
 func WithRefresh(hook func(email string) error) func(*FakeIDP) {
@@ -753,7 +762,16 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
 	mux.Get("/.well-known/openid-configuration", func(rw http.ResponseWriter, r *http.Request) {
 		f.logger.Info(r.Context(), "http OIDC config", slogRequestFields(r)...)
 
-		_ = json.NewEncoder(rw).Encode(f.provider)
+		cpy := f.provider
+		if f.hookWellKnown != nil {
+			err := f.hookWellKnown(r, &cpy)
+			if err != nil {
+				http.Error(rw, err.Error(), http.StatusInternalServerError)
+				return
+			}
+		}
+
+		_ = json.NewEncoder(rw).Encode(cpy)
 	})
 
 	// Authorize is called when the user is redirected to the IDP to login.
@@ -1371,8 +1389,11 @@ func (f *FakeIDP) AppCredentials() (clientID string, clientSecret string) {
 	return f.clientID, f.clientSecret
 }
 
-// OIDCConfig returns the OIDC config to use for Coderd.
-func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
+func (f *FakeIDP) PublicKey() crypto.PublicKey {
+	return f.key.Public()
+}
+
+func (f *FakeIDP) OauthConfig(t testing.TB, scopes []string) *oauth2.Config {
 	t.Helper()
 
 	if len(scopes) == 0 {
@@ -1391,22 +1412,50 @@ func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *co
 		RedirectURL: "https://redirect.com",
 		Scopes:      scopes,
 	}
+	f.cfg = oauthCfg
 
-	ctx := oidc.ClientContext(context.Background(), f.HTTPClient(nil))
+	return oauthCfg
+}
+
+func (f *FakeIDP) OIDCConfigSkipIssuerChecks(t testing.TB, scopes []string, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
+	ctx := oidc.InsecureIssuerURLContext(context.Background(), f.issuer)
+
+	return f.internalOIDCConfig(ctx, t, scopes, func(config *oidc.Config) {
+		config.SkipIssuerCheck = true
+	}, opts...)
+}
+
+func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
+	return f.internalOIDCConfig(context.Background(), t, scopes, nil, opts...)
+}
+
+// OIDCConfig returns the OIDC config to use for Coderd.
+func (f *FakeIDP) internalOIDCConfig(ctx context.Context, t testing.TB, scopes []string, verifierOpt func(config *oidc.Config), opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
+	t.Helper()
+
+	oauthCfg := f.OauthConfig(t, scopes)
+
+	ctx = oidc.ClientContext(ctx, f.HTTPClient(nil))
 	p, err := oidc.NewProvider(ctx, f.provider.Issuer)
 	require.NoError(t, err, "failed to create OIDC provider")
+
+	verifierConfig := &oidc.Config{
+		ClientID: oauthCfg.ClientID,
+		SupportedSigningAlgs: []string{
+			"RS256",
+		},
+		// Todo: add support for Now()
+	}
+	if verifierOpt != nil {
+		verifierOpt(verifierConfig)
+	}
+
 	cfg := &coderd.OIDCConfig{
 		OAuth2Config: oauthCfg,
 		Provider:     p,
 		Verifier: oidc.NewVerifier(f.provider.Issuer, &oidc.StaticKeySet{
 			PublicKeys: []crypto.PublicKey{f.key.Public()},
-		}, &oidc.Config{
-			ClientID: oauthCfg.ClientID,
-			SupportedSigningAlgs: []string{
-				"RS256",
-			},
-			// Todo: add support for Now()
-		}),
+		}, verifierConfig),
 		UsernameField: "preferred_username",
 		EmailField:    "email",
 		AuthURLParams: map[string]string{"access_type": "offline"},
@@ -1419,13 +1468,12 @@ func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *co
 		opt(cfg)
 	}
 
-	f.cfg = oauthCfg
 	return cfg
 }
 
 func (f *FakeIDP) getClaims(m *syncmap.Map[string, jwt.MapClaims], key string) (jwt.MapClaims, bool) {
 	v, ok := m.Load(key)
-	if !ok {
+	if !ok || v == nil {
 		if f.defaultIDClaims != nil {
 			return f.defaultIDClaims, true
 		}

diff --git a/coderd/coderdtest/oidctest/idp_test.go b/coderd/coderdtest/oidctest/idp_test.go
@@ -2,19 +2,22 @@ package oidctest_test
 
 import (
 	"context"
+	"crypto"
 	"net/http"
-	"net/http/httptest"
 	"testing"
 	"time"
 
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/stretchr/testify/assert"
+	"golang.org/x/xerrors"
 
 	"github.com/coreos/go-oidc/v3/oidc"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2"
 
+	"github.com/coder/coder/v2/coderd"
 	"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
+	"github.com/coder/coder/v2/testutil"
 )
 
 // TestFakeIDPBasicFlow tests the basic flow of the fake IDP.
@@ -27,12 +30,6 @@ func TestFakeIDPBasicFlow(t *testing.T) {
 		oidctest.WithLogging(t, nil),
 	)
 
-	var handler http.Handler
-	srv := httptest.NewServer(http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		handler.ServeHTTP(w, r)
-	})))
-	defer srv.Close()
-
 	cfg := fake.OIDCConfig(t, nil)
 	cli := fake.HTTPClient(nil)
 	ctx := oidc.ClientContext(context.Background(), cli)
@@ -71,3 +68,84 @@ func TestFakeIDPBasicFlow(t *testing.T) {
 	require.NoError(t, err, "failed to refresh token")
 	require.NotEmpty(t, refreshed.AccessToken, "access token is empty on refresh")
 }
+
+// TestIDPIssuerMismatch emulates a situation where the IDP issuer url does
+// not match the one in the well-known config and claims.
+// This can happen in some edge cases and in some azure configurations.
+//
+// This test just makes sure a fake IDP can set up this scenario.
+func TestIDPIssuerMismatch(t *testing.T) {
+	t.Parallel()
+
+	const proxyURL = "https://proxy.com"
+	const primaryURL = "https://primary.com"
+
+	fake := oidctest.NewFakeIDP(t,
+		oidctest.WithIssuer(proxyURL),
+		oidctest.WithDefaultIDClaims(jwt.MapClaims{
+			"iss": primaryURL,
+		}),
+		oidctest.WithHookWellKnown(func(r *http.Request, j *oidctest.ProviderJSON) error {
+			// host should be proxy.com, but we return the primaryURL
+			if r.Host != "proxy.com" {
+				return xerrors.Errorf("unexpected host: %s", r.Host)
+			}
+			j.Issuer = primaryURL
+			return nil
+		}),
+		oidctest.WithLogging(t, nil),
+	)
+
+	ctx := testutil.Context(t, testutil.WaitMedium)
+	// Do not use real network requests
+	cli := fake.HTTPClient(nil)
+	ctx = oidc.ClientContext(ctx, cli)
+
+	// Allow the issuer mismatch
+	verifierContext := oidc.InsecureIssuerURLContext(ctx, "this field does not matter")
+	p, err := oidc.NewProvider(verifierContext, "https://proxy.com")
+	require.NoError(t, err, "failed to create OIDC provider")
+
+	oauthConfig := fake.OauthConfig(t, nil)
+	cfg := &coderd.OIDCConfig{
+		OAuth2Config: oauthConfig,
+		Provider:     p,
+		Verifier: oidc.NewVerifier(fake.WellknownConfig().Issuer, &oidc.StaticKeySet{
+			PublicKeys: []crypto.PublicKey{fake.PublicKey()},
+		}, &oidc.Config{
+			SkipIssuerCheck: true,
+			ClientID:        oauthConfig.ClientID,
+			SupportedSigningAlgs: []string{
+				"RS256",
+			},
+		}),
+		UsernameField: "preferred_username",
+		EmailField:    "email",
+		AuthURLParams: map[string]string{"access_type": "offline"},
+	}
+
+	const expectedState = "random-state"
+	var token *oauth2.Token
+
+	fake.SetCoderdCallbackHandler(func(w http.ResponseWriter, r *http.Request) {
+		// Emulate OIDC flow
+		code := r.URL.Query().Get("code")
+		state := r.URL.Query().Get("state")
+		assert.Equal(t, expectedState, state, "state mismatch")
+
+		oauthToken, err := cfg.Exchange(ctx, code)
+		if assert.NoError(t, err, "failed to exchange code") {
+			assert.NotEmpty(t, oauthToken.AccessToken, "access token is empty")
+			assert.NotEmpty(t, oauthToken.RefreshToken, "refresh token is empty")
+		}
+		token = oauthToken
+	})
+
+	//nolint:bodyclose
+	resp := fake.OIDCCallback(t, expectedState, nil) // Use default claims
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+
+	idToken, err := cfg.Verifier.Verify(ctx, token.Extra("id_token").(string))
+	require.NoError(t, err)
+	require.Equal(t, primaryURL, idToken.Issuer)
+}