From 50b78e332548beda353f7da357c4c79e98e01161 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Jan 2024 09:13:30 -0600 Subject: [PATCH] chore: instrument external oauth2 requests (#11519) * chore: instrument external oauth2 requests External requests made by oauth2 configs are now instrumented into prometheus metrics. --- cli/create_test.go | 10 +- cli/server.go | 24 ++- coderd/coderdtest/oidctest/idp.go | 52 +++++- coderd/externalauth/externalauth.go | 57 +++--- coderd/externalauth/externalauth_test.go | 21 ++- coderd/externalauth_test.go | 44 ++--- coderd/httpmw/apikey.go | 7 +- coderd/httpmw/oauth2.go | 11 +- coderd/oauthpki/oidcpki.go | 8 +- coderd/promoauth/doc.go | 4 + coderd/promoauth/oauth2.go | 173 ++++++++++++++++++ coderd/promoauth/oauth2_test.go | 80 ++++++++ .../provisionerdserver/provisionerdserver.go | 8 +- .../provisionerdserver_test.go | 4 +- coderd/templateversions_test.go | 8 +- coderd/userauth.go | 5 +- testutil/oauth2.go | 7 + 17 files changed, 425 insertions(+), 98 deletions(-) create mode 100644 coderd/promoauth/doc.go create mode 100644 coderd/promoauth/oauth2.go create mode 100644 coderd/promoauth/oauth2_test.go diff --git a/cli/create_test.go b/cli/create_test.go index 42b526d404..903694167f 100644 --- a/cli/create_test.go +++ b/cli/create_test.go @@ -767,11 +767,11 @@ func TestCreateWithGitAuth(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ ExternalAuthConfigs: []*externalauth.Config{{ - OAuth2Config: &testutil.OAuth2Config{}, - ID: "github", - Regex: regexp.MustCompile(`github\.com`), - Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), - DisplayName: "GitHub", + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + DisplayName: "GitHub", }}, IncludeProvisionerDaemon: true, }) diff --git a/cli/server.go b/cli/server.go index 72c72679fd..dd45e4dea8 100644 --- a/cli/server.go +++ b/cli/server.go @@ -80,6 +80,7 @@ import ( "github.com/coder/coder/v2/coderd/oauthpki" "github.com/coder/coder/v2/coderd/prometheusmetrics" "github.com/coder/coder/v2/coderd/prometheusmetrics/insights" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/coderd/schedule" "github.com/coder/coder/v2/coderd/telemetry" "github.com/coder/coder/v2/coderd/tracing" @@ -133,7 +134,7 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co Scopes: vals.OIDC.Scopes, } - var useCfg httpmw.OAuth2Config = oauthCfg + var useCfg promoauth.OAuth2Config = oauthCfg if vals.OIDC.ClientKeyFile != "" { // PKI authentication is done in the params. If a // 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. return xerrors.Errorf("read external auth providers from env: %w", err) } + promRegistry := prometheus.NewRegistry() + oauthInstrument := promoauth.NewFactory(promRegistry) vals.ExternalAuthConfigs.Value = append(vals.ExternalAuthConfigs.Value, extAuthEnv...) externalAuthConfigs, err := externalauth.ConvertConfig( + oauthInstrument, vals.ExternalAuthConfigs.Value, vals.AccessURL.Value(), ) @@ -571,7 +575,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. // the DeploymentValues instead, this just serves to indicate the source of each // option. This is just defensive to prevent accidentally leaking. DeploymentOptions: codersdk.DeploymentOptionsWithoutSecrets(opts), - PrometheusRegistry: prometheus.NewRegistry(), + PrometheusRegistry: promRegistry, APIRateLimit: int(vals.RateLimit.API.Value()), LoginRateLimit: loginRateLimit, FilesRateLimit: filesRateLimit, @@ -617,7 +621,9 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. } if vals.OAuth2.Github.ClientSecret != "" { - options.GithubOAuth2Config, err = configureGithubOAuth2(vals.AccessURL.Value(), + options.GithubOAuth2Config, err = configureGithubOAuth2( + oauthInstrument, + vals.AccessURL.Value(), vals.OAuth2.Github.ClientID.String(), vals.OAuth2.Github.ClientSecret.String(), vals.OAuth2.Github.AllowSignups.Value(), @@ -636,6 +642,12 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. 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, vals) if err != nil { return xerrors.Errorf("create oidc config: %w", err) @@ -1737,7 +1749,7 @@ func configureCAPool(tlsClientCAFile string, tlsConfig *tls.Config) error { } //nolint:revive // Ignore flag-parameter: parameter 'allowEveryone' seems to be a control flag, avoid control coupling (revive) -func configureGithubOAuth2(accessURL *url.URL, clientID, clientSecret string, allowSignups, allowEveryone bool, allowOrgs []string, rawTeams []string, enterpriseBaseURL string) (*coderd.GithubOAuth2Config, error) { +func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, clientID, clientSecret string, allowSignups, allowEveryone bool, allowOrgs []string, rawTeams []string, enterpriseBaseURL string) (*coderd.GithubOAuth2Config, error) { redirectURL, err := accessURL.Parse("/api/v2/users/oauth2/github/callback") if err != nil { return nil, xerrors.Errorf("parse github oauth callback url: %w", err) @@ -1790,7 +1802,7 @@ func configureGithubOAuth2(accessURL *url.URL, clientID, clientSecret string, al } return &coderd.GithubOAuth2Config{ - OAuth2Config: &oauth2.Config{ + OAuth2Config: instrument.New("github-login", &oauth2.Config{ ClientID: clientID, ClientSecret: clientSecret, Endpoint: endpoint, @@ -1800,7 +1812,7 @@ func configureGithubOAuth2(accessURL *url.URL, clientID, clientSecret string, al "read:org", "user:email", }, - }, + }), AllowSignups: allowSignups, AllowEveryone: allowEveryone, AllowOrganizations: allowOrgs, diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index 20702be16a..460c687a97 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -24,6 +24,7 @@ import ( "github.com/go-jose/go-jose/v3" "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" @@ -33,6 +34,7 @@ import ( "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/externalauth" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/coderd/util/syncmap" "github.com/coder/coder/v2/codersdk" ) @@ -223,6 +225,10 @@ func (f *FakeIDP) WellknownConfig() ProviderJSON { return f.provider } +func (f *FakeIDP) IssuerURL() *url.URL { + return f.issuerURL +} + func (f *FakeIDP) updateIssuerURL(t testing.TB, issuer string) { t.Helper() @@ -397,6 +403,44 @@ func (f *FakeIDP) ExternalLogin(t testing.TB, client *codersdk.Client, opts ...f _ = res.Body.Close() } +// CreateAuthCode emulates a user clicking "allow" on the IDP page. When doing +// unit tests, it's easier to skip this step sometimes. It does make an actual +// request to the IDP, so it should be equivalent to doing this "manually" with +// actual requests. +func (f *FakeIDP) CreateAuthCode(t testing.TB, state string, opts ...func(r *http.Request)) string { + // We need to store some claims, because this is also an OIDC provider, and + // it expects some claims to be present. + f.stateToIDTokenClaims.Store(state, jwt.MapClaims{}) + + u := f.cfg.AuthCodeURL(state) + r, err := http.NewRequestWithContext(context.Background(), http.MethodPost, u, nil) + require.NoError(t, err, "failed to create auth request") + + for _, opt := range opts { + opt(r) + } + + rw := httptest.NewRecorder() + f.handler.ServeHTTP(rw, r) + resp := rw.Result() + defer resp.Body.Close() + + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode, "expected redirect") + to := resp.Header.Get("Location") + require.NotEmpty(t, to, "expected redirect location") + + toURL, err := url.Parse(to) + require.NoError(t, err, "failed to parse redirect location") + + code := toURL.Query().Get("code") + require.NotEmpty(t, code, "expected code in redirect location") + + newState := toURL.Query().Get("state") + require.Equal(t, state, newState, "expected state to match") + + return code +} + // OIDCCallback will emulate the IDP redirecting back to the Coder callback. // This is helpful if no Coderd exists because the IDP needs to redirect to // something. @@ -901,9 +945,10 @@ func (f *FakeIDP) ExternalAuthConfig(t testing.TB, id string, custom *ExternalAu handle(email, rw, r) } } + instrumentF := promoauth.NewFactory(prometheus.NewRegistry()) cfg := &externalauth.Config{ - OAuth2Config: f.OIDCConfig(t, nil), - ID: id, + InstrumentedOAuth2Config: instrumentF.New(f.clientID, f.OIDCConfig(t, nil)), + ID: id, // No defaults for these fields by omitting the type Type: "", DisplayIcon: f.WellknownConfig().UserInfoURL, @@ -920,10 +965,10 @@ func (f *FakeIDP) ExternalAuthConfig(t testing.TB, id string, custom *ExternalAu // 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 { t.Helper() + if len(scopes) == 0 { scopes = []string{"openid", "email", "profile"} } - oauthCfg := &oauth2.Config{ ClientID: f.clientID, ClientSecret: f.clientSecret, @@ -966,7 +1011,6 @@ func (f *FakeIDP) OIDCConfig(t testing.TB, scopes []string, opts ...func(cfg *co } f.cfg = oauthCfg - return cfg } diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 9243aa29e4..09d7d829ba 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -22,19 +22,14 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/codersdk" "github.com/coder/retry" ) -type OAuth2Config interface { - AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string - Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) - TokenSource(context.Context, *oauth2.Token) oauth2.TokenSource -} - // Config is used for authentication for Git operations. type Config struct { - OAuth2Config + promoauth.InstrumentedOAuth2Config // ID is a unique identifier for the authenticator. ID string // Type is the type of provider. @@ -192,12 +187,8 @@ func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *coders return false, nil, err } - cli := http.DefaultClient - if v, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok { - cli = v - } req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) - res, err := cli.Do(req) + res, err := c.InstrumentedOAuth2Config.Do(ctx, promoauth.SourceValidateToken, req) if err != nil { return false, nil, err } @@ -247,7 +238,7 @@ func (c *Config) AppInstallations(ctx context.Context, token string) ([]codersdk return nil, false, err } req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) - res, err := http.DefaultClient.Do(req) + res, err := c.InstrumentedOAuth2Config.Do(ctx, promoauth.SourceAppInstallations, req) if err != nil { return nil, false, err } @@ -287,6 +278,8 @@ func (c *Config) AppInstallations(ctx context.Context, token string) ([]codersdk } type DeviceAuth struct { + // Config is provided for the http client method. + Config promoauth.InstrumentedOAuth2Config ClientID string TokenURL string Scopes []string @@ -307,8 +300,17 @@ func (c *DeviceAuth) AuthorizeDevice(ctx context.Context) (*codersdk.ExternalAut if err != nil { return nil, err } + + do := http.DefaultClient.Do + if c.Config != nil { + // The cfg can be nil in unit tests. + do = func(req *http.Request) (*http.Response, error) { + return c.Config.Do(ctx, promoauth.SourceAuthorizeDevice, req) + } + } + + resp, err := do(req) req.Header.Set("Accept", "application/json") - resp, err := http.DefaultClient.Do(req) if err != nil { return nil, err } @@ -401,7 +403,7 @@ func (c *DeviceAuth) formatDeviceCodeURL() (string, error) { // ConvertConfig converts the SDK configuration entry format // to the parsed and ready-to-consume in coderd provider type. -func ConvertConfig(entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([]*Config, error) { +func ConvertConfig(instrument *promoauth.Factory, entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([]*Config, error) { ids := map[string]struct{}{} configs := []*Config{} for _, entry := range entries { @@ -453,7 +455,7 @@ func ConvertConfig(entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([ Scopes: entry.Scopes, } - var oauthConfig OAuth2Config = oc + var oauthConfig promoauth.OAuth2Config = oc // Azure DevOps uses JWT token authentication! if entry.Type == string(codersdk.EnhancedExternalAuthProviderAzureDevops) { oauthConfig = &jwtConfig{oc} @@ -463,17 +465,17 @@ func ConvertConfig(entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([ } cfg := &Config{ - OAuth2Config: oauthConfig, - ID: entry.ID, - Regex: regex, - Type: entry.Type, - NoRefresh: entry.NoRefresh, - ValidateURL: entry.ValidateURL, - AppInstallationsURL: entry.AppInstallationsURL, - AppInstallURL: entry.AppInstallURL, - DisplayName: entry.DisplayName, - DisplayIcon: entry.DisplayIcon, - ExtraTokenKeys: entry.ExtraTokenKeys, + InstrumentedOAuth2Config: instrument.New(entry.ID, oauthConfig), + ID: entry.ID, + Regex: regex, + Type: entry.Type, + NoRefresh: entry.NoRefresh, + ValidateURL: entry.ValidateURL, + AppInstallationsURL: entry.AppInstallationsURL, + AppInstallURL: entry.AppInstallURL, + DisplayName: entry.DisplayName, + DisplayIcon: entry.DisplayIcon, + ExtraTokenKeys: entry.ExtraTokenKeys, } if entry.DeviceFlow { @@ -481,6 +483,7 @@ func ConvertConfig(entries []codersdk.ExternalAuthConfig, accessURL *url.URL) ([ return nil, xerrors.Errorf("external auth provider %q: device auth url must be provided", entry.ID) } cfg.DeviceAuth = &DeviceAuth{ + Config: cfg, ClientID: entry.ClientID, TokenURL: oc.Endpoint.TokenURL, Scopes: entry.Scopes, diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 387bdc7738..84fbe4ff5d 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -12,6 +12,7 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" "golang.org/x/oauth2" "golang.org/x/xerrors" @@ -22,6 +23,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/coderd/externalauth" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" ) @@ -94,7 +96,7 @@ func TestRefreshToken(t *testing.T) { t.Run("FalseIfTokenSourceFails", func(t *testing.T) { t.Parallel() config := &externalauth.Config{ - OAuth2Config: &testutil.OAuth2Config{ + InstrumentedOAuth2Config: &testutil.OAuth2Config{ TokenSourceFunc: func() (*oauth2.Token, error) { return nil, xerrors.New("failure") }, @@ -301,9 +303,10 @@ func TestRefreshToken(t *testing.T) { func TestExchangeWithClientSecret(t *testing.T) { t.Parallel() + instrument := promoauth.NewFactory(prometheus.NewRegistry()) // This ensures a provider that requires the custom // client secret exchange works. - configs, err := externalauth.ConvertConfig([]codersdk.ExternalAuthConfig{{ + configs, err := externalauth.ConvertConfig(instrument, []codersdk.ExternalAuthConfig{{ // JFrog just happens to require this custom type. Type: codersdk.EnhancedExternalAuthProviderJFrog.String(), @@ -335,6 +338,8 @@ func TestExchangeWithClientSecret(t *testing.T) { func TestConvertYAML(t *testing.T) { t.Parallel() + + instrument := promoauth.NewFactory(prometheus.NewRegistry()) for _, tc := range []struct { Name string Input []codersdk.ExternalAuthConfig @@ -387,7 +392,7 @@ func TestConvertYAML(t *testing.T) { tc := tc t.Run(tc.Name, func(t *testing.T) { t.Parallel() - output, err := externalauth.ConvertConfig(tc.Input, &url.URL{}) + output, err := externalauth.ConvertConfig(instrument, tc.Input, &url.URL{}) if tc.Error != "" { require.Error(t, err) require.Contains(t, err.Error(), tc.Error) @@ -399,7 +404,7 @@ func TestConvertYAML(t *testing.T) { t.Run("CustomScopesAndEndpoint", func(t *testing.T) { t.Parallel() - config, err := externalauth.ConvertConfig([]codersdk.ExternalAuthConfig{{ + config, err := externalauth.ConvertConfig(instrument, []codersdk.ExternalAuthConfig{{ Type: string(codersdk.EnhancedExternalAuthProviderGitLab), ClientID: "id", ClientSecret: "secret", @@ -433,10 +438,12 @@ func setupOauth2Test(t *testing.T, settings testConfig) (*oidctest.FakeIDP, *ext append([]oidctest.FakeIDPOpt{}, settings.FakeIDPOpts...)..., ) + f := promoauth.NewFactory(prometheus.NewRegistry()) config := &externalauth.Config{ - OAuth2Config: fake.OIDCConfig(t, nil, settings.CoderOIDCConfigOpts...), - ID: providerID, - ValidateURL: fake.WellknownConfig().UserInfoURL, + InstrumentedOAuth2Config: f.New("test-oauth2", + fake.OIDCConfig(t, nil, settings.CoderOIDCConfigOpts...)), + ID: providerID, + ValidateURL: fake.WellknownConfig().UserInfoURL, } settings.ExternalAuthOpt(config) diff --git a/coderd/externalauth_test.go b/coderd/externalauth_test.go index 34c1fe7bcd..1d0b06bbc0 100644 --- a/coderd/externalauth_test.go +++ b/coderd/externalauth_test.go @@ -316,10 +316,10 @@ func TestExternalAuthCallback(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, ExternalAuthConfigs: []*externalauth.Config{{ - OAuth2Config: &testutil.OAuth2Config{}, - ID: "github", - Regex: regexp.MustCompile(`github\.com`), - Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), }}, }) user := coderdtest.CreateFirstUser(t, client) @@ -347,10 +347,10 @@ func TestExternalAuthCallback(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, ExternalAuthConfigs: []*externalauth.Config{{ - OAuth2Config: &testutil.OAuth2Config{}, - ID: "github", - Regex: regexp.MustCompile(`github\.com`), - Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), }}, }) resp := coderdtest.RequestExternalAuthCallback(t, "github", client) @@ -361,10 +361,10 @@ func TestExternalAuthCallback(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, ExternalAuthConfigs: []*externalauth.Config{{ - OAuth2Config: &testutil.OAuth2Config{}, - ID: "github", - Regex: regexp.MustCompile(`github\.com`), - Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), }}, }) _ = coderdtest.CreateFirstUser(t, client) @@ -387,11 +387,11 @@ func TestExternalAuthCallback(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, ExternalAuthConfigs: []*externalauth.Config{{ - ValidateURL: srv.URL, - OAuth2Config: &testutil.OAuth2Config{}, - ID: "github", - Regex: regexp.MustCompile(`github\.com`), - Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + ValidateURL: srv.URL, + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), }}, }) user := coderdtest.CreateFirstUser(t, client) @@ -443,7 +443,7 @@ func TestExternalAuthCallback(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, ExternalAuthConfigs: []*externalauth.Config{{ - OAuth2Config: &testutil.OAuth2Config{ + InstrumentedOAuth2Config: &testutil.OAuth2Config{ Token: &oauth2.Token{ AccessToken: "token", RefreshToken: "something", @@ -497,10 +497,10 @@ func TestExternalAuthCallback(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, ExternalAuthConfigs: []*externalauth.Config{{ - OAuth2Config: &testutil.OAuth2Config{}, - ID: "github", - Regex: regexp.MustCompile(`github\.com`), - Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), }}, }) user := coderdtest.CreateFirstUser(t, client) diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index dfffe9cf09..46d8c97014 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -22,6 +22,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" ) @@ -74,8 +75,8 @@ func UserAuthorization(r *http.Request) Authorization { // OAuth2Configs is a collection of configurations for OAuth-based authentication. // This should be extended to support other authentication types in the future. type OAuth2Configs struct { - Github OAuth2Config - OIDC OAuth2Config + Github promoauth.OAuth2Config + OIDC promoauth.OAuth2Config } func (c *OAuth2Configs) IsZero() bool { @@ -270,7 +271,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon }) } - var oauthConfig OAuth2Config + var oauthConfig promoauth.OAuth2Config switch key.LoginType { case database.LoginTypeGithub: oauthConfig = cfg.OAuth2Configs.Github diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index c300576aa8..dbb763bc9d 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -10,6 +10,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/cryptorand" ) @@ -22,14 +23,6 @@ type OAuth2State struct { StateString string } -// OAuth2Config exposes a subset of *oauth2.Config functions for easier testing. -// *oauth2.Config should be used instead of implementing this in production. -type OAuth2Config interface { - AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string - Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) - TokenSource(context.Context, *oauth2.Token) oauth2.TokenSource -} - // OAuth2 returns the state from an oauth request. func OAuth2(r *http.Request) OAuth2State { oauth, ok := r.Context().Value(oauth2StateKey{}).(OAuth2State) @@ -44,7 +37,7 @@ func OAuth2(r *http.Request) OAuth2State { // a "code" URL parameter will be redirected. // AuthURLOpts are passed to the AuthCodeURL function. If this is nil, // the default option oauth2.AccessTypeOffline will be used. -func ExtractOAuth2(config OAuth2Config, client *http.Client, authURLOpts map[string]string) func(http.Handler) http.Handler { +func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOpts map[string]string) func(http.Handler) http.Handler { opts := make([]oauth2.AuthCodeOption, 0, len(authURLOpts)+1) opts = append(opts, oauth2.AccessTypeOffline) for k, v := range authURLOpts { diff --git a/coderd/oauthpki/oidcpki.go b/coderd/oauthpki/oidcpki.go index c44d130e5b..d761c43e44 100644 --- a/coderd/oauthpki/oidcpki.go +++ b/coderd/oauthpki/oidcpki.go @@ -20,7 +20,7 @@ import ( "golang.org/x/oauth2/jws" "golang.org/x/xerrors" - "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/promoauth" ) // Config uses jwt assertions over client_secret for oauth2 authentication of @@ -33,7 +33,7 @@ import ( // // https://datatracker.ietf.org/doc/html/rfc7523 type Config struct { - cfg httpmw.OAuth2Config + cfg promoauth.OAuth2Config // These values should match those provided in the oauth2.Config. // Because the inner config is an interface, we need to duplicate these @@ -57,7 +57,7 @@ type ConfigParams struct { PemEncodedKey []byte PemEncodedCert []byte - Config httpmw.OAuth2Config + Config promoauth.OAuth2Config } // NewOauth2PKIConfig creates the oauth2 config for PKI based auth. It requires the certificate and it's private key. @@ -180,6 +180,8 @@ func (src *jwtTokenSource) Token() (*oauth2.Token, error) { } cli := http.DefaultClient if v, ok := src.ctx.Value(oauth2.HTTPClient).(*http.Client); ok { + // This client should be the instrumented client already. So no need to + // handle this manually. cli = v } diff --git a/coderd/promoauth/doc.go b/coderd/promoauth/doc.go new file mode 100644 index 0000000000..72f30b48cf --- /dev/null +++ b/coderd/promoauth/doc.go @@ -0,0 +1,4 @@ +// Package promoauth is for instrumenting oauth2 flows with prometheus metrics. +// Specifically, it is intended to count the number of external requests made +// by the underlying oauth2 exchanges. +package promoauth diff --git a/coderd/promoauth/oauth2.go b/coderd/promoauth/oauth2.go new file mode 100644 index 0000000000..d3d168b8ec --- /dev/null +++ b/coderd/promoauth/oauth2.go @@ -0,0 +1,173 @@ +package promoauth + +import ( + "context" + "fmt" + "net/http" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promauto" + "golang.org/x/oauth2" +) + +type Oauth2Source string + +const ( + SourceValidateToken Oauth2Source = "ValidateToken" + SourceExchange Oauth2Source = "Exchange" + SourceTokenSource Oauth2Source = "TokenSource" + SourceAppInstallations Oauth2Source = "AppInstallations" + SourceAuthorizeDevice Oauth2Source = "AuthorizeDevice" +) + +// OAuth2Config exposes a subset of *oauth2.Config functions for easier testing. +// *oauth2.Config should be used instead of implementing this in production. +type OAuth2Config interface { + AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string + Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) + TokenSource(context.Context, *oauth2.Token) oauth2.TokenSource +} + +// InstrumentedOAuth2Config extends OAuth2Config with a `Do` method that allows +// external oauth related calls to be instrumented. This is to support +// "ValidateToken" which is not an oauth2 specified method. +// These calls still count against the api rate limit, and should be instrumented. +type InstrumentedOAuth2Config interface { + OAuth2Config + + // Do is provided as a convenience method to make a request with the oauth2 client. + // It mirrors `http.Client.Do`. + Do(ctx context.Context, source Oauth2Source, req *http.Request) (*http.Response, error) +} + +var _ OAuth2Config = (*Config)(nil) + +// Factory allows us to have 1 set of metrics for all oauth2 providers. +// Primarily to avoid any prometheus errors registering duplicate metrics. +type Factory struct { + metrics *metrics +} + +// metrics is the reusable metrics for all oauth2 providers. +type metrics struct { + externalRequestCount *prometheus.CounterVec +} + +func NewFactory(registry prometheus.Registerer) *Factory { + factory := promauto.With(registry) + + return &Factory{ + metrics: &metrics{ + externalRequestCount: factory.NewCounterVec(prometheus.CounterOpts{ + Namespace: "coderd", + Subsystem: "oauth2", + Name: "external_requests_total", + Help: "The total number of api calls made to external oauth2 providers. 'status_code' will be 0 if the request failed with no response.", + }, []string{ + "name", + "source", + "status_code", + }), + }, + } +} + +func (f *Factory) New(name string, under OAuth2Config) *Config { + return &Config{ + name: name, + underlying: under, + metrics: f.metrics, + } +} + +type Config struct { + // Name is a human friendly name to identify the oauth2 provider. This should be + // deterministic from restart to restart, as it is going to be used as a label in + // prometheus metrics. + name string + underlying OAuth2Config + metrics *metrics +} + +func (c *Config) Do(ctx context.Context, source Oauth2Source, req *http.Request) (*http.Response, error) { + cli := c.oauthHTTPClient(ctx, source) + return cli.Do(req) +} + +func (c *Config) AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string { + // No external requests are made when constructing the auth code url. + return c.underlying.AuthCodeURL(state, opts...) +} + +func (c *Config) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { + return c.underlying.Exchange(c.wrapClient(ctx, SourceExchange), code, opts...) +} + +func (c *Config) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.TokenSource { + return c.underlying.TokenSource(c.wrapClient(ctx, SourceTokenSource), token) +} + +// wrapClient is the only way we can accurately instrument the oauth2 client. +// This is because method calls to the 'OAuth2Config' interface are not 1:1 with +// network requests. +// +// For example, the 'TokenSource' method will return a token +// source that will make a network request when the 'Token' method is called on +// it if the token is expired. +func (c *Config) wrapClient(ctx context.Context, source Oauth2Source) context.Context { + return context.WithValue(ctx, oauth2.HTTPClient, c.oauthHTTPClient(ctx, source)) +} + +// oauthHTTPClient returns an http client that will instrument every request made. +func (c *Config) oauthHTTPClient(ctx context.Context, source Oauth2Source) *http.Client { + cli := &http.Client{} + + // Check if the context has a http client already. + if hc, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok { + cli = hc + } + + // The new tripper will instrument every request made by the oauth2 client. + cli.Transport = newInstrumentedTripper(c, source, cli.Transport) + return cli +} + +type instrumentedTripper struct { + c *Config + source Oauth2Source + underlying http.RoundTripper +} + +// newInstrumentedTripper intercepts a http request, and increments the +// externalRequestCount metric. +func newInstrumentedTripper(c *Config, source Oauth2Source, under http.RoundTripper) *instrumentedTripper { + if under == nil { + under = http.DefaultTransport + } + + // If the underlying transport is the default, we need to clone it. + // We should also clone it if it supports cloning. + if tr, ok := under.(*http.Transport); ok { + under = tr.Clone() + } + + return &instrumentedTripper{ + c: c, + source: source, + underlying: under, + } +} + +func (i *instrumentedTripper) RoundTrip(r *http.Request) (*http.Response, error) { + resp, err := i.underlying.RoundTrip(r) + var statusCode int + if resp != nil { + statusCode = resp.StatusCode + } + i.c.metrics.externalRequestCount.With(prometheus.Labels{ + "name": i.c.name, + "source": string(i.source), + "status_code": fmt.Sprintf("%d", statusCode), + }).Inc() + return resp, err +} diff --git a/coderd/promoauth/oauth2_test.go b/coderd/promoauth/oauth2_test.go new file mode 100644 index 0000000000..b9c72f95a3 --- /dev/null +++ b/coderd/promoauth/oauth2_test.go @@ -0,0 +1,80 @@ +package promoauth_test + +import ( + "net/http" + "net/url" + "testing" + "time" + + "github.com/prometheus/client_golang/prometheus" + ptestutil "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/coderdtest/oidctest" + "github.com/coder/coder/v2/coderd/externalauth" + "github.com/coder/coder/v2/coderd/promoauth" + "github.com/coder/coder/v2/testutil" +) + +func TestInstrument(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + idp := oidctest.NewFakeIDP(t, oidctest.WithServing()) + reg := prometheus.NewRegistry() + count := func() int { + return ptestutil.CollectAndCount(reg, "coderd_oauth2_external_requests_total") + } + + factory := promoauth.NewFactory(reg) + const id = "test" + cfg := externalauth.Config{ + InstrumentedOAuth2Config: factory.New(id, idp.OIDCConfig(t, []string{})), + ID: "test", + ValidateURL: must[*url.URL](t)(idp.IssuerURL().Parse("/oauth2/userinfo")).String(), + } + + // 0 Requests before we start + require.Equal(t, count(), 0) + + // Exchange should trigger a request + code := idp.CreateAuthCode(t, "foo") + token, err := cfg.Exchange(ctx, code) + require.NoError(t, err) + require.Equal(t, count(), 1) + + // Force a refresh + token.Expiry = time.Now().Add(time.Hour * -1) + src := cfg.TokenSource(ctx, token) + refreshed, err := src.Token() + require.NoError(t, err) + require.NotEqual(t, token.AccessToken, refreshed.AccessToken, "token refreshed") + require.Equal(t, count(), 2) + + // Try a validate + valid, _, err := cfg.ValidateToken(ctx, refreshed.AccessToken) + require.NoError(t, err) + require.True(t, valid) + require.Equal(t, count(), 3) + + // Verify the default client was not broken. This check is added because we + // extend the http.DefaultTransport. If a `.Clone()` is not done, this can be + // mis-used. It is cheap to run this quick check. + req, err := http.NewRequestWithContext(ctx, http.MethodGet, + must[*url.URL](t)(idp.IssuerURL().Parse("/.well-known/openid-configuration")).String(), nil) + require.NoError(t, err) + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + _ = resp.Body.Close() + + require.Equal(t, count(), 3) +} + +func must[V any](t *testing.T) func(v V, err error) V { + return func(v V, err error) V { + t.Helper() + require.NoError(t, err) + return v + } +} diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 2715ba6776..1330f370d6 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -32,7 +32,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/pubsub" "github.com/coder/coder/v2/coderd/externalauth" - "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/coderd/schedule" "github.com/coder/coder/v2/coderd/telemetry" "github.com/coder/coder/v2/coderd/tracing" @@ -55,7 +55,7 @@ const ( ) type Options struct { - OIDCConfig httpmw.OAuth2Config + OIDCConfig promoauth.OAuth2Config ExternalAuthConfigs []*externalauth.Config // TimeNowFn is only used in tests TimeNowFn func() time.Time @@ -96,7 +96,7 @@ type server struct { UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore] DeploymentValues *codersdk.DeploymentValues - OIDCConfig httpmw.OAuth2Config + OIDCConfig promoauth.OAuth2Config TimeNowFn func() time.Time @@ -1736,7 +1736,7 @@ func deleteSessionToken(ctx context.Context, db database.Store, workspace databa // obtainOIDCAccessToken returns a valid OpenID Connect access token // for the user if it's able to obtain one, otherwise it returns an empty string. -func obtainOIDCAccessToken(ctx context.Context, db database.Store, oidcConfig httpmw.OAuth2Config, userID uuid.UUID) (string, error) { +func obtainOIDCAccessToken(ctx context.Context, db database.Store, oidcConfig promoauth.OAuth2Config, userID uuid.UUID) (string, error) { link, err := db.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{ UserID: userID, LoginType: database.LoginTypeOIDC, diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 915b50a31d..01a0837a4d 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -187,8 +187,8 @@ func TestAcquireJob(t *testing.T) { srv, db, ps, _ := setup(t, false, &overrides{ deploymentValues: dv, externalAuthConfigs: []*externalauth.Config{{ - ID: gitAuthProvider, - OAuth2Config: &testutil.OAuth2Config{}, + ID: gitAuthProvider, + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, }}, }) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index b7765f076b..4423bbc4e7 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -335,10 +335,10 @@ func TestTemplateVersionsExternalAuth(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, ExternalAuthConfigs: []*externalauth.Config{{ - OAuth2Config: &testutil.OAuth2Config{}, - ID: "github", - Regex: regexp.MustCompile(`github\.com`), - Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), }}, }) user := coderdtest.CreateFirstUser(t, client) diff --git a/coderd/userauth.go b/coderd/userauth.go index 94fe821da7..54f10d7388 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -31,6 +31,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/userpassword" "github.com/coder/coder/v2/codersdk" @@ -438,7 +439,7 @@ type GithubOAuth2Team struct { // GithubOAuth2Provider exposes required functions for the Github authentication flow. type GithubOAuth2Config struct { - httpmw.OAuth2Config + promoauth.OAuth2Config AuthenticatedUser func(ctx context.Context, client *http.Client) (*github.User, error) ListEmails func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) ListOrganizationMemberships func(ctx context.Context, client *http.Client) ([]*github.Membership, error) @@ -662,7 +663,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { } type OIDCConfig struct { - httpmw.OAuth2Config + promoauth.OAuth2Config Provider *oidc.Provider Verifier *oidc.IDTokenVerifier diff --git a/testutil/oauth2.go b/testutil/oauth2.go index e152caf956..196e2e7bf7 100644 --- a/testutil/oauth2.go +++ b/testutil/oauth2.go @@ -2,10 +2,13 @@ package testutil import ( "context" + "net/http" "net/url" "time" "golang.org/x/oauth2" + + "github.com/coder/coder/v2/coderd/promoauth" ) type OAuth2Config struct { @@ -13,6 +16,10 @@ type OAuth2Config struct { TokenSourceFunc OAuth2TokenSource } +func (*OAuth2Config) Do(_ context.Context, _ promoauth.Oauth2Source, req *http.Request) (*http.Response, error) { + return http.DefaultClient.Do(req) +} + func (*OAuth2Config) AuthCodeURL(state string, _ ...oauth2.AuthCodeOption) string { return "/?state=" + url.QueryEscape(state) }