Skip to content

Commit 50b78e3

Browse files
EmyrkEmyrk
authored
chore: instrument external oauth2 requests (#11519)
* chore: instrument external oauth2 requests External requests made by oauth2 configs are now instrumented into prometheus metrics.
1 parent aa7fe07 commit 50b78e3

17 files changed

+425
-98
lines changed

‎cli/create_test.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -767,11 +767,11 @@ func TestCreateWithGitAuth(t *testing.T) {
767767

768768
client := coderdtest.New(t, &coderdtest.Options{
769769
ExternalAuthConfigs: []*externalauth.Config{{
770-
OAuth2Config: &testutil.OAuth2Config{},
771-
ID: "github",
772-
Regex: regexp.MustCompile(`github\.com`),
773-
Type: codersdk.EnhancedExternalAuthProviderGitHub.String(),
774-
DisplayName: "GitHub",
770+
InstrumentedOAuth2Config: &testutil.OAuth2Config{},
771+
ID: "github",
772+
Regex: regexp.MustCompile(`github\.com`),
773+
Type: codersdk.EnhancedExternalAuthProviderGitHub.String(),
774+
DisplayName: "GitHub",
775775
}},
776776
IncludeProvisionerDaemon: true,
777777
})

‎cli/server.go

+18-6
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ import (
8080
"github.com/coder/coder/v2/coderd/oauthpki"
8181
"github.com/coder/coder/v2/coderd/prometheusmetrics"
8282
"github.com/coder/coder/v2/coderd/prometheusmetrics/insights"
83+
"github.com/coder/coder/v2/coderd/promoauth"
8384
"github.com/coder/coder/v2/coderd/schedule"
8485
"github.com/coder/coder/v2/coderd/telemetry"
8586
"github.com/coder/coder/v2/coderd/tracing"
@@ -133,7 +134,7 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co
133134
Scopes: vals.OIDC.Scopes,
134135
}
135136

136-
var useCfg httpmw.OAuth2Config = oauthCfg
137+
var useCfg promoauth.OAuth2Config = oauthCfg
137138
if vals.OIDC.ClientKeyFile != "" {
138139
// PKI authentication is done in the params. If a
139140
// counter example is found, we can add a config option to
@@ -523,8 +524,11 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
523524
return xerrors.Errorf("read external auth providers from env: %w", err)
524525
}
525526

527+
promRegistry := prometheus.NewRegistry()
528+
oauthInstrument := promoauth.NewFactory(promRegistry)
526529
vals.ExternalAuthConfigs.Value = append(vals.ExternalAuthConfigs.Value, extAuthEnv...)
527530
externalAuthConfigs, err := externalauth.ConvertConfig(
531+
oauthInstrument,
528532
vals.ExternalAuthConfigs.Value,
529533
vals.AccessURL.Value(),
530534
)
@@ -571,7 +575,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
571575
// the DeploymentValues instead, this just serves to indicate the source of each
572576
// option. This is just defensive to prevent accidentally leaking.
573577
DeploymentOptions: codersdk.DeploymentOptionsWithoutSecrets(opts),
574-
PrometheusRegistry: prometheus.NewRegistry(),
578+
PrometheusRegistry: promRegistry,
575579
APIRateLimit: int(vals.RateLimit.API.Value()),
576580
LoginRateLimit: loginRateLimit,
577581
FilesRateLimit: filesRateLimit,
@@ -617,7 +621,9 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
617621
}
618622

619623
if vals.OAuth2.Github.ClientSecret != "" {
620-
options.GithubOAuth2Config, err = configureGithubOAuth2(vals.AccessURL.Value(),
624+
options.GithubOAuth2Config, err = configureGithubOAuth2(
625+
oauthInstrument,
626+
vals.AccessURL.Value(),
621627
vals.OAuth2.Github.ClientID.String(),
622628
vals.OAuth2.Github.ClientSecret.String(),
623629
vals.OAuth2.Github.AllowSignups.Value(),
@@ -636,6 +642,12 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
636642
logger.Warn(ctx, "coder will not check email_verified for OIDC logins")
637643
}
638644

645+
// This OIDC config is **not** being instrumented with the
646+
// oauth2 instrument wrapper. If we implement the missing
647+
// oidc methods, then we can instrument it.
648+
// Missing:
649+
// - Userinfo
650+
// - Verify
639651
oc, err := createOIDCConfig(ctx, vals)
640652
if err != nil {
641653
return xerrors.Errorf("create oidc config: %w", err)
@@ -1737,7 +1749,7 @@ func configureCAPool(tlsClientCAFile string, tlsConfig *tls.Config) error {
17371749
}
17381750

17391751
//nolint:revive // Ignore flag-parameter: parameter 'allowEveryone' seems to be a control flag, avoid control coupling (revive)
1740-
func configureGithubOAuth2(accessURL *url.URL, clientID, clientSecret string, allowSignups, allowEveryone bool, allowOrgs []string, rawTeams []string, enterpriseBaseURL string) (*coderd.GithubOAuth2Config, error) {
1752+
func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, clientID, clientSecret string, allowSignups, allowEveryone bool, allowOrgs []string, rawTeams []string, enterpriseBaseURL string) (*coderd.GithubOAuth2Config, error) {
17411753
redirectURL, err := accessURL.Parse("/api/v2/users/oauth2/github/callback")
17421754
if err != nil {
17431755
return nil, xerrors.Errorf("parse github oauth callback url: %w", err)
@@ -1790,7 +1802,7 @@ func configureGithubOAuth2(accessURL *url.URL, clientID, clientSecret string, al
17901802
}
17911803

17921804
return &coderd.GithubOAuth2Config{
1793-
OAuth2Config: &oauth2.Config{
1805+
OAuth2Config: instrument.New("github-login", &oauth2.Config{
17941806
ClientID: clientID,
17951807
ClientSecret: clientSecret,
17961808
Endpoint: endpoint,
@@ -1800,7 +1812,7 @@ func configureGithubOAuth2(accessURL *url.URL, clientID, clientSecret string, al
18001812
"read:org",
18011813
"user:email",
18021814
},
1803-
},
1815+
}),
18041816
AllowSignups: allowSignups,
18051817
AllowEveryone: allowEveryone,
18061818
AllowOrganizations: allowOrgs,

‎coderd/coderdtest/oidctest/idp.go

+48-4
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"github.com/go-jose/go-jose/v3"
2525
"github.com/golang-jwt/jwt/v4"
2626
"github.com/google/uuid"
27+
"github.com/prometheus/client_golang/prometheus"
2728
"github.com/stretchr/testify/assert"
2829
"github.com/stretchr/testify/require"
2930
"golang.org/x/oauth2"
@@ -33,6 +34,7 @@ import (
3334
"cdr.dev/slog/sloggers/slogtest"
3435
"github.com/coder/coder/v2/coderd"
3536
"github.com/coder/coder/v2/coderd/externalauth"
37+
"github.com/coder/coder/v2/coderd/promoauth"
3638
"github.com/coder/coder/v2/coderd/util/syncmap"
3739
"github.com/coder/coder/v2/codersdk"
3840
)
@@ -223,6 +225,10 @@ func (f *FakeIDP) WellknownConfig() ProviderJSON {
223225
return f.provider
224226
}
225227

228+
func (f *FakeIDP) IssuerURL() *url.URL {
229+
return f.issuerURL
230+
}
231+
226232
func (f *FakeIDP) updateIssuerURL(t testing.TB, issuer string) {
227233
t.Helper()
228234

@@ -397,6 +403,44 @@ func (f *FakeIDP) ExternalLogin(t testing.TB, client *codersdk.Client, opts ...f
397403
_ = res.Body.Close()
398404
}
399405

406+
// CreateAuthCode emulates a user clicking "allow" on the IDP page. When doing
407+
// unit tests, it's easier to skip this step sometimes. It does make an actual
408+
// request to the IDP, so it should be equivalent to doing this "manually" with
409+
// actual requests.
410+
func (f *FakeIDP) CreateAuthCode(t testing.TB, state string, opts ...func(r *http.Request)) string {
411+
// We need to store some claims, because this is also an OIDC provider, and
412+
// it expects some claims to be present.
413+
f.stateToIDTokenClaims.Store(state, jwt.MapClaims{})
414+
415+
u := f.cfg.AuthCodeURL(state)
416+
r, err := http.NewRequestWithContext(context.Background(), http.MethodPost, u, nil)
417+
require.NoError(t, err, "failed to create auth request")
418+
419+
for _, opt := range opts {
420+
opt(r)
421+
}
422+
423+
rw := httptest.NewRecorder()
424+
f.handler.ServeHTTP(rw, r)
425+
resp := rw.Result()
426+
defer resp.Body.Close()
427+
428+
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode, "expected redirect")
429+
to := resp.Header.Get("Location")
430+
require.NotEmpty(t, to, "expected redirect location")
431+
432+
toURL, err := url.Parse(to)
433+
require.NoError(t, err, "failed to parse redirect location")
434+
435+
code := toURL.Query().Get("code")
436+
require.NotEmpty(t, code, "expected code in redirect location")
437+
438+
newState := toURL.Query().Get("state")
439+
require.Equal(t, state, newState, "expected state to match")
440+
441+
return code
442+
}
443+
400444
// OIDCCallback will emulate the IDP redirecting back to the Coder callback.
401445
// This is helpful if no Coderd exists because the IDP needs to redirect to
402446
// something.
@@ -901,9 +945,10 @@ func (f *FakeIDP) ExternalAuthConfig(t testing.TB, id string, custom *ExternalAu
901945
handle(email, rw, r)
902946
}
903947
}
948+
instrumentF := promoauth.NewFactory(prometheus.NewRegistry())
904949
cfg := &externalauth.Config{
905-
OAuth2Config: f.OIDCConfig(t, nil),
906-
ID: id,
950+
InstrumentedOAuth2Config: instrumentF.New(f.clientID, f.OIDCConfig(t, nil)),
951+
ID: id,
907952
// No defaults for these fields by omitting the type
908953
Type: "",
909954
DisplayIcon: f.WellknownConfig().UserInfoURL,
@@ -920,10 +965,10 @@ func (f *FakeIDP) ExternalAuthConfig(t testing.TB, id string, custom *ExternalAu
920965
// OIDCConfig returns the OIDC config to use for Coderd.
921966
func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
922967
t.Helper()
968+
923969
if len(scopes) == 0 {
924970
scopes = []string{"openid", "email", "profile"}
925971
}
926-
927972
oauthCfg := &oauth2.Config{
928973
ClientID: f.clientID,
929974
ClientSecret: f.clientSecret,
@@ -966,7 +1011,6 @@ func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *co
9661011
}
9671012

9681013
f.cfg = oauthCfg
969-
9701014
return cfg
9711015
}
9721016

‎coderd/externalauth/externalauth.go

+30-27
Original file line numberDiff line numberDiff line change
@@ -22,19 +22,14 @@ import (
2222
"github.com/coder/coder/v2/coderd/database"
2323
"github.com/coder/coder/v2/coderd/database/dbtime"
2424
"github.com/coder/coder/v2/coderd/httpapi"
25+
"github.com/coder/coder/v2/coderd/promoauth"
2526
"github.com/coder/coder/v2/codersdk"
2627
"github.com/coder/retry"
2728
)
2829

29-
type OAuth2Config interface {
30-
AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string
31-
Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error)
32-
TokenSource(context.Context, *oauth2.Token) oauth2.TokenSource
33-
}
34-
3530
// Config is used for authentication for Git operations.
3631
type Config struct {
37-
OAuth2Config
32+
promoauth.InstrumentedOAuth2Config
3833
// ID is a unique identifier for the authenticator.
3934
ID string
4035
// Type is the type of provider.
@@ -192,12 +187,8 @@ func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *coders
192187
return false, nil, err
193188
}
194189

195-
cli := http.DefaultClient
196-
if v, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok {
197-
cli = v
198-
}
199190
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
200-
res, err := cli.Do(req)
191+
res, err := c.InstrumentedOAuth2Config.Do(ctx, promoauth.SourceValidateToken, req)
201192
if err != nil {
202193
return false, nil, err
203194
}
@@ -247,7 +238,7 @@ func (c *Config) AppInstallations(ctx context.Context, token string) ([]codersdk
247238
return nil, false, err
248239
}
249240
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
250-
res, err := http.DefaultClient.Do(req)
241+
res, err := c.InstrumentedOAuth2Config.Do(ctx, promoauth.SourceAppInstallations, req)
251242
if err != nil {
252243
return nil, false, err
253244
}
@@ -287,6 +278,8 @@ func (c *Config) AppInstallations(ctx context.Context, token string) ([]codersdk
287278
}
288279

289280
type DeviceAuth struct {
281+
// Config is provided for the http client method.
282+
Config promoauth.InstrumentedOAuth2Config
290283
ClientID string
291284
TokenURL string
292285
Scopes []string
@@ -307,8 +300,17 @@ func (c *DeviceAuth) AuthorizeDevice(ctx context.Context) (*codersdk.ExternalAut
307300
if err != nil {
308301
return nil, err
309302
}
303+
304+
do := http.DefaultClient.Do
305+
if c.Config != nil {
306+
// The cfg can be nil in unit tests.
307+
do = func(req *http.Request) (*http.Response, error) {
308+
return c.Config.Do(ctx, promoauth.SourceAuthorizeDevice, req)
309+
}
310+
}
311+
312+
resp, err := do(req)
310313
req.Header.Set("Accept", "application/json")
311-
resp, err := http.DefaultClient.Do(req)
312314
if err != nil {
313315
return nil, err
314316
}
@@ -401,7 +403,7 @@ func (c *DeviceAuth) formatDeviceCodeURL() (string, error) {
401403

402404
// ConvertConfig converts the SDK configuration entry format
403405
// to the parsed and ready-to-consume in coderd provider type.
404-
func ConvertConfig(entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([]*Config, error) {
406+
func ConvertConfig(instrument *promoauth.Factory, entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([]*Config, error) {
405407
ids := map[string]struct{}{}
406408
configs := []*Config{}
407409
for _, entry := range entries {
@@ -453,7 +455,7 @@ func ConvertConfig(entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([
453455
Scopes: entry.Scopes,
454456
}
455457

456-
var oauthConfig OAuth2Config = oc
458+
var oauthConfig promoauth.OAuth2Config = oc
457459
// Azure DevOps uses JWT token authentication!
458460
if entry.Type == string(codersdk.EnhancedExternalAuthProviderAzureDevops) {
459461
oauthConfig = &jwtConfig{oc}
@@ -463,24 +465,25 @@ func ConvertConfig(entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([
463465
}
464466

465467
cfg := &Config{
466-
OAuth2Config: oauthConfig,
467-
ID: entry.ID,
468-
Regex: regex,
469-
Type: entry.Type,
470-
NoRefresh: entry.NoRefresh,
471-
ValidateURL: entry.ValidateURL,
472-
AppInstallationsURL: entry.AppInstallationsURL,
473-
AppInstallURL: entry.AppInstallURL,
474-
DisplayName: entry.DisplayName,
475-
DisplayIcon: entry.DisplayIcon,
476-
ExtraTokenKeys: entry.ExtraTokenKeys,
468+
InstrumentedOAuth2Config: instrument.New(entry.ID, oauthConfig),
469+
ID: entry.ID,
470+
Regex: regex,
471+
Type: entry.Type,
472+
NoRefresh: entry.NoRefresh,
473+
ValidateURL: entry.ValidateURL,
474+
AppInstallationsURL: entry.AppInstallationsURL,
475+
AppInstallURL: entry.AppInstallURL,
476+
DisplayName: entry.DisplayName,
477+
DisplayIcon: entry.DisplayIcon,
478+
ExtraTokenKeys: entry.ExtraTokenKeys,
477479
}
478480

479481
if entry.DeviceFlow {
480482
if entry.DeviceCodeURL == "" {
481483
return nil, xerrors.Errorf("external auth provider %q: device auth url must be provided", entry.ID)
482484
}
483485
cfg.DeviceAuth = &DeviceAuth{
486+
Config: cfg,
484487
ClientID: entry.ClientID,
485488
TokenURL: oc.Endpoint.TokenURL,
486489
Scopes: entry.Scopes,

0 commit comments

Comments
 (0)