Skip to content

Commit 4f01372

Browse files
EmyrkEmyrk
authored
feat: implement disabling oidc issuer checks (coder#13991)
* use DANGEROUS prefix and drop a warning log
1 parent 652827f commit 4f01372

File tree

14 files changed

+272
-22
lines changed

14 files changed

+272
-22
lines changed

‎cli/server.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,14 +106,20 @@ import (
106106
"github.com/coder/coder/v2/tailnet"
107107
)
108108

109-
func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) {
109+
func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) {
110110
if vals.OIDC.ClientID == "" {
111111
return nil, xerrors.Errorf("OIDC client ID must be set!")
112112
}
113113
if vals.OIDC.IssuerURL == "" {
114114
return nil, xerrors.Errorf("OIDC issuer URL must be set!")
115115
}
116116

117+
// Skipping issuer checks is not recommended.
118+
if vals.OIDC.SkipIssuerChecks {
119+
logger.Warn(ctx, "issuer checks with OIDC is disabled. This is not recommended as it can compromise the security of the authentication")
120+
ctx = oidc.InsecureIssuerURLContext(ctx, vals.OIDC.IssuerURL.String())
121+
}
122+
117123
oidcProvider, err := oidc.NewProvider(
118124
ctx, vals.OIDC.IssuerURL.String(),
119125
)
@@ -167,6 +173,9 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co
167173
Provider: oidcProvider,
168174
Verifier: oidcProvider.Verifier(&oidc.Config{
169175
ClientID: vals.OIDC.ClientID.String(),
176+
// Enabling this skips checking the "iss" claim in the token
177+
// matches the issuer URL. This is not recommended.
178+
SkipIssuerCheck: vals.OIDC.SkipIssuerChecks.Value(),
170179
}),
171180
EmailDomain: vals.OIDC.EmailDomain,
172181
AllowSignups: vals.OIDC.AllowSignups.Value(),
@@ -657,7 +666,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
657666
// Missing:
658667
// - Userinfo
659668
// - Verify
660-
oc, err := createOIDCConfig(ctx, vals)
669+
oc, err := createOIDCConfig(ctx, options.Logger, vals)
661670
if err != nil {
662671
return xerrors.Errorf("create oidc config: %w", err)
663672
}

‎cli/testdata/coder_server_--help.golden

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,12 @@ OIDC OPTIONS:
513513
The custom text to show on the error page informing about disabled
514514
OIDC signups. Markdown format is supported.
515515

516+
--dangerous-oidc-skip-issuer-checks bool, $CODER_DANGEROUS_OIDC_SKIP_ISSUER_CHECKS
517+
OIDC issuer urls must match in the request, the id_token 'iss' claim,
518+
and in the well-known configuration. This flag disables that
519+
requirement, and can lead to an insecure OIDC configuration. It is not
520+
recommended to use this flag.
521+
516522
PROVISIONING OPTIONS:
517523
Tune the behavior of the provisioner, which is responsible for creating,
518524
updating, and deleting workspace resources.

‎cli/testdata/server-config.yaml.golden

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,11 @@ oidc:
364364
# Markdown format is supported.
365365
# (default: <unset>, type: string)
366366
signupsDisabledText: ""
367+
# OIDC issuer urls must match in the request, the id_token 'iss' claim, and in the
368+
# well-known configuration. This flag disables that requirement, and can lead to
369+
# an insecure OIDC configuration. It is not recommended to use this flag.
370+
# (default: <unset>, type: bool)
371+
dangerousSkipIssuerChecks: false
367372
# Telemetry is critical to our ability to improve Coder. We strip all personal
368373
# information before sending data to our servers. Please only disable telemetry
369374
# when required by your organization's security policy.

‎coderd/apidoc/docs.go

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 61 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,9 @@ type FakeIDP struct {
9797
deviceCode *syncmap.Map[string, deviceFlow]
9898

9999
// hooks
100+
// hookWellKnown allows mutating the returned .well-known/configuration JSON.
101+
// Using this can break the IDP configuration, so be careful.
102+
hookWellKnown func(r *http.Request, j *ProviderJSON) error
100103
// hookValidRedirectURL can be used to reject a redirect url from the
101104
// IDP -> Application. Almost all IDPs have the concept of
102105
// "Authorized Redirect URLs". This can be used to emulate that.
@@ -151,6 +154,12 @@ func WithMiddlewares(mws ...func(http.Handler) http.Handler) func(*FakeIDP) {
151154
}
152155
}
153156

157+
func WithHookWellKnown(hook func(r *http.Request, j *ProviderJSON) error) func(*FakeIDP) {
158+
return func(f *FakeIDP) {
159+
f.hookWellKnown = hook
160+
}
161+
}
162+
154163
// WithRefresh is called when a refresh token is used. The email is
155164
// the email of the user that is being refreshed assuming the claims are correct.
156165
func WithRefresh(hook func(email string) error) func(*FakeIDP) {
@@ -753,7 +762,16 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
753762
mux.Get("/.well-known/openid-configuration", func(rw http.ResponseWriter, r *http.Request) {
754763
f.logger.Info(r.Context(), "http OIDC config", slogRequestFields(r)...)
755764

756-
_ = json.NewEncoder(rw).Encode(f.provider)
765+
cpy := f.provider
766+
if f.hookWellKnown != nil {
767+
err := f.hookWellKnown(r, &cpy)
768+
if err != nil {
769+
http.Error(rw, err.Error(), http.StatusInternalServerError)
770+
return
771+
}
772+
}
773+
774+
_ = json.NewEncoder(rw).Encode(cpy)
757775
})
758776

759777
// 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) {
13711389
return f.clientID, f.clientSecret
13721390
}
13731391

1374-
// OIDCConfig returns the OIDC config to use for Coderd.
1375-
func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
1392+
func (f *FakeIDP) PublicKey() crypto.PublicKey {
1393+
return f.key.Public()
1394+
}
1395+
1396+
func (f *FakeIDP) OauthConfig(t testing.TB, scopes []string) *oauth2.Config {
13761397
t.Helper()
13771398

13781399
if len(scopes) == 0 {
@@ -1391,22 +1412,50 @@ func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *co
13911412
RedirectURL: "https://redirect.com",
13921413
Scopes: scopes,
13931414
}
1415+
f.cfg = oauthCfg
13941416

1395-
ctx := oidc.ClientContext(context.Background(), f.HTTPClient(nil))
1417+
return oauthCfg
1418+
}
1419+
1420+
func (f *FakeIDP) OIDCConfigSkipIssuerChecks(t testing.TB, scopes []string, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
1421+
ctx := oidc.InsecureIssuerURLContext(context.Background(), f.issuer)
1422+
1423+
return f.internalOIDCConfig(ctx, t, scopes, func(config *oidc.Config) {
1424+
config.SkipIssuerCheck = true
1425+
}, opts...)
1426+
}
1427+
1428+
func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
1429+
return f.internalOIDCConfig(context.Background(), t, scopes, nil, opts...)
1430+
}
1431+
1432+
// OIDCConfig returns the OIDC config to use for Coderd.
1433+
func (f *FakeIDP) internalOIDCConfig(ctx context.Context, t testing.TB, scopes []string, verifierOpt func(config *oidc.Config), opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
1434+
t.Helper()
1435+
1436+
oauthCfg := f.OauthConfig(t, scopes)
1437+
1438+
ctx = oidc.ClientContext(ctx, f.HTTPClient(nil))
13961439
p, err := oidc.NewProvider(ctx, f.provider.Issuer)
13971440
require.NoError(t, err, "failed to create OIDC provider")
1441+
1442+
verifierConfig := &oidc.Config{
1443+
ClientID: oauthCfg.ClientID,
1444+
SupportedSigningAlgs: []string{
1445+
"RS256",
1446+
},
1447+
// Todo: add support for Now()
1448+
}
1449+
if verifierOpt != nil {
1450+
verifierOpt(verifierConfig)
1451+
}
1452+
13981453
cfg := &coderd.OIDCConfig{
13991454
OAuth2Config: oauthCfg,
14001455
Provider: p,
14011456
Verifier: oidc.NewVerifier(f.provider.Issuer, &oidc.StaticKeySet{
14021457
PublicKeys: []crypto.PublicKey{f.key.Public()},
1403-
}, &oidc.Config{
1404-
ClientID: oauthCfg.ClientID,
1405-
SupportedSigningAlgs: []string{
1406-
"RS256",
1407-
},
1408-
// Todo: add support for Now()
1409-
}),
1458+
}, verifierConfig),
14101459
UsernameField: "preferred_username",
14111460
EmailField: "email",
14121461
AuthURLParams: map[string]string{"access_type": "offline"},
@@ -1419,13 +1468,12 @@ func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *co
14191468
opt(cfg)
14201469
}
14211470

1422-
f.cfg = oauthCfg
14231471
return cfg
14241472
}
14251473

14261474
func (f *FakeIDP) getClaims(m *syncmap.Map[string, jwt.MapClaims], key string) (jwt.MapClaims, bool) {
14271475
v, ok := m.Load(key)
1428-
if !ok {
1476+
if !ok || v == nil {
14291477
if f.defaultIDClaims != nil {
14301478
return f.defaultIDClaims, true
14311479
}

‎coderd/coderdtest/oidctest/idp_test.go

Lines changed: 85 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,22 @@ package oidctest_test
22

33
import (
44
"context"
5+
"crypto"
56
"net/http"
6-
"net/http/httptest"
77
"testing"
88
"time"
99

1010
"github.com/golang-jwt/jwt/v4"
1111
"github.com/stretchr/testify/assert"
12+
"golang.org/x/xerrors"
1213

1314
"github.com/coreos/go-oidc/v3/oidc"
1415
"github.com/stretchr/testify/require"
1516
"golang.org/x/oauth2"
1617

18+
"github.com/coder/coder/v2/coderd"
1719
"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
20+
"github.com/coder/coder/v2/testutil"
1821
)
1922

2023
// TestFakeIDPBasicFlow tests the basic flow of the fake IDP.
@@ -27,12 +30,6 @@ func TestFakeIDPBasicFlow(t *testing.T) {
2730
oidctest.WithLogging(t, nil),
2831
)
2932

30-
var handler http.Handler
31-
srv := httptest.NewServer(http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
32-
handler.ServeHTTP(w, r)
33-
})))
34-
defer srv.Close()
35-
3633
cfg := fake.OIDCConfig(t, nil)
3734
cli := fake.HTTPClient(nil)
3835
ctx := oidc.ClientContext(context.Background(), cli)
@@ -71,3 +68,84 @@ func TestFakeIDPBasicFlow(t *testing.T) {
7168
require.NoError(t, err, "failed to refresh token")
7269
require.NotEmpty(t, refreshed.AccessToken, "access token is empty on refresh")
7370
}
71+
72+
// TestIDPIssuerMismatch emulates a situation where the IDP issuer url does
73+
// not match the one in the well-known config and claims.
74+
// This can happen in some edge cases and in some azure configurations.
75+
//
76+
// This test just makes sure a fake IDP can set up this scenario.
77+
func TestIDPIssuerMismatch(t *testing.T) {
78+
t.Parallel()
79+
80+
const proxyURL = "https://proxy.com"
81+
const primaryURL = "https://primary.com"
82+
83+
fake := oidctest.NewFakeIDP(t,
84+
oidctest.WithIssuer(proxyURL),
85+
oidctest.WithDefaultIDClaims(jwt.MapClaims{
86+
"iss": primaryURL,
87+
}),
88+
oidctest.WithHookWellKnown(func(r *http.Request, j *oidctest.ProviderJSON) error {
89+
// host should be proxy.com, but we return the primaryURL
90+
if r.Host != "proxy.com" {
91+
return xerrors.Errorf("unexpected host: %s", r.Host)
92+
}
93+
j.Issuer = primaryURL
94+
return nil
95+
}),
96+
oidctest.WithLogging(t, nil),
97+
)
98+
99+
ctx := testutil.Context(t, testutil.WaitMedium)
100+
// Do not use real network requests
101+
cli := fake.HTTPClient(nil)
102+
ctx = oidc.ClientContext(ctx, cli)
103+
104+
// Allow the issuer mismatch
105+
verifierContext := oidc.InsecureIssuerURLContext(ctx, "this field does not matter")
106+
p, err := oidc.NewProvider(verifierContext, "https://proxy.com")
107+
require.NoError(t, err, "failed to create OIDC provider")
108+
109+
oauthConfig := fake.OauthConfig(t, nil)
110+
cfg := &coderd.OIDCConfig{
111+
OAuth2Config: oauthConfig,
112+
Provider: p,
113+
Verifier: oidc.NewVerifier(fake.WellknownConfig().Issuer, &oidc.StaticKeySet{
114+
PublicKeys: []crypto.PublicKey{fake.PublicKey()},
115+
}, &oidc.Config{
116+
SkipIssuerCheck: true,
117+
ClientID: oauthConfig.ClientID,
118+
SupportedSigningAlgs: []string{
119+
"RS256",
120+
},
121+
}),
122+
UsernameField: "preferred_username",
123+
EmailField: "email",
124+
AuthURLParams: map[string]string{"access_type": "offline"},
125+
}
126+
127+
const expectedState = "random-state"
128+
var token *oauth2.Token
129+
130+
fake.SetCoderdCallbackHandler(func(w http.ResponseWriter, r *http.Request) {
131+
// Emulate OIDC flow
132+
code := r.URL.Query().Get("code")
133+
state := r.URL.Query().Get("state")
134+
assert.Equal(t, expectedState, state, "state mismatch")
135+
136+
oauthToken, err := cfg.Exchange(ctx, code)
137+
if assert.NoError(t, err, "failed to exchange code") {
138+
assert.NotEmpty(t, oauthToken.AccessToken, "access token is empty")
139+
assert.NotEmpty(t, oauthToken.RefreshToken, "refresh token is empty")
140+
}
141+
token = oauthToken
142+
})
143+
144+
//nolint:bodyclose
145+
resp := fake.OIDCCallback(t, expectedState, nil) // Use default claims
146+
require.Equal(t, http.StatusOK, resp.StatusCode)
147+
148+
idToken, err := cfg.Verifier.Verify(ctx, token.Extra("id_token").(string))
149+
require.NoError(t, err)
150+
require.Equal(t, primaryURL, idToken.Issuer)
151+
}

0 commit comments

Comments
 (0)