diff --git a/coderd/oauth2.go b/coderd/oauth2.go index fb82781eb5e..6f85538e359 100644 --- a/coderd/oauth2.go +++ b/coderd/oauth2.go @@ -154,7 +154,7 @@ func (api *API) postOAuth2ProviderAppAuthorize() http.HandlerFunc { // @Success 200 {object} codersdk.OAuth2TokenResponse // @Router /oauth2/tokens [post] func (api *API) postOAuth2ProviderAppToken() http.HandlerFunc { - return oauth2provider.Tokens(api.Database, api.DeploymentValues.Sessions) + return oauth2provider.Tokens(api.Database, api.DeploymentValues.Sessions, api.Logger) } // @Summary Delete OAuth2 application tokens. diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 2bb7bbb9717..36e09778612 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -28,8 +28,8 @@ import ( "github.com/coder/coder/v2/site" ) -// Rejection reasons from negotiateScope, rendered into error_description. Each -// is wrapped as `%q: %w` with the offending value: xerrors repeats the +// Rejection reasons from scope negotiation, rendered into error_description. +// Each is wrapped as `%q: %w` with the offending value: xerrors repeats the // sentinel's own text unless %w is the final verb. var ( // A requested name outside the external scope catalog: unrecognized, or @@ -70,6 +70,50 @@ func noScopeAllowlist(appScope sql.NullString) bool { return !appScope.Valid || appScope.String == "" } +// grantableScopes narrows an app's registered allowlist to the catalog names +// this deployment offers, which only ever narrows what can be granted. An empty +// result is returned rather than rejected: negotiation and redemption report it +// differently. +func grantableScopes(appScope string) []string { + allowed := strings.Fields(appScope) + filtered := make([]string, 0, len(allowed)) + for _, a := range allowed { + if rbac.IsExternalScope(rbac.ScopeName(a)) { + filtered = append(filtered, a) + } + } + // Canonicalized so both sides expand: rbac.ExpandScope knows `coder:all` + // and not the `all` alias that IsExternalScope accepts. + return canonicalScopes(filtered) +} + +// firstScopeOutsideAllowlist returns the first scope in granted that the +// allowlist does not confer, or "" when it confers all of them. The check is +// coverage rather than membership: an app allowed `coder:workspaces.access` +// covers `workspace:read`. Both arguments must already be canonical, and an +// undecidable comparison refuses rather than grants. +func firstScopeOutsideAllowlist(ctx context.Context, logger slog.Logger, app database.OAuth2ProviderApp, allowlist, granted []string) (string, error) { + allowedNames := make([]rbac.ScopeName, 0, len(allowlist)) + for _, a := range allowlist { + allowedNames = append(allowedNames, rbac.ScopeName(a)) + } + for _, s := range granted { + covered, err := rbac.ScopesCover(allowedNames, rbac.ScopeName(s)) + if err != nil { + logger.Warn(ctx, "oauth2 scope coverage could not be determined", + slog.Error(err), + slog.F("app_id", app.ID.String()), + slog.F("app_scope", app.Scope.String), + slog.F("scope", s)) + return "", xerrors.Errorf("%q: %w", s, errCoverageUndecidable) + } + if !covered { + return s, nil + } + } + return "", nil +} + // negotiateScope decides the scope the authorization code will carry. Every // requested name must be in the external scope catalog and covered by the app's // allowlist. A rejection is an RFC 6749 §4.1.2.1 invalid_scope. @@ -104,51 +148,24 @@ func negotiateScope(ctx context.Context, logger slog.Logger, app database.OAuth2 return strings.Join(granted, " "), nil } - // The stored allowlist may name a scope since removed from the catalog, or - // never in it. Filtering only ever narrows what is granted. - allowed := strings.Fields(app.Scope.String) - filtered := make([]string, 0, len(allowed)) - for _, a := range allowed { - if rbac.IsExternalScope(rbac.ScopeName(a)) { - filtered = append(filtered, a) - } - } - if len(filtered) == 0 { + allowlist := grantableScopes(app.Scope.String) + if len(allowlist) == 0 { // Rejected rather than read as absent, which would grant more than this // allowlist ever permitted. The stored value is named verbatim so a // whitespace-only allowlist does not render as "". return "", xerrors.Errorf("%q: %w", app.Scope.String, errNoGrantableScope) } - // Canonicalized so both sides expand: rbac.ExpandScope knows `coder:all` - // and not the `all` alias that IsExternalScope accepts. - filtered = canonicalScopes(filtered) if len(requested) == 0 { - return strings.Join(filtered, " "), nil // RFC 6749 §3.3 default + return strings.Join(allowlist, " "), nil // RFC 6749 §3.3 default } - // The allowlist is a ceiling on authority, not a menu of spellings, so the - // check is coverage rather than membership: an app allowed - // `coder:workspaces.access` can approve a request for `workspace:read`. - allowedNames := make([]rbac.ScopeName, 0, len(filtered)) - for _, a := range filtered { - allowedNames = append(allowedNames, rbac.ScopeName(a)) + outside, err := firstScopeOutsideAllowlist(ctx, logger, app, allowlist, granted) + if err != nil { + return "", err } - for _, s := range granted { - covered, err := rbac.ScopesCover(allowedNames, rbac.ScopeName(s)) - if err != nil { - // Refuse rather than grant on an incomplete comparison. The - // underlying error names RBAC internals, so it goes to the log. - logger.Warn(ctx, "oauth2 scope coverage could not be determined", - slog.Error(err), - slog.F("app_id", app.ID.String()), - slog.F("app_scope", app.Scope.String), - slog.F("requested_scope", s)) - return "", xerrors.Errorf("%q: %w", s, errCoverageUndecidable) - } - if !covered { - return "", xerrors.Errorf("%q: %w", s, errScopeNotAllowed) - } + if outside != "" { + return "", xerrors.Errorf("%q: %w", outside, errScopeNotAllowed) } return strings.Join(granted, " "), nil } diff --git a/coderd/oauth2provider/authorize_internal_test.go b/coderd/oauth2provider/authorize_internal_test.go index 7bedac932fa..55fbedca68c 100644 --- a/coderd/oauth2provider/authorize_internal_test.go +++ b/coderd/oauth2provider/authorize_internal_test.go @@ -254,6 +254,7 @@ var ( ReasonUnknownScope = errUnknownScope.Error() ReasonNoGrantableScope = errNoGrantableScope.Error() ReasonScopeNotAllowed = errScopeNotAllowed.Error() + ReasonStaleScope = errStaleScope.Error() ) func TestNoScopeAllowlist(t *testing.T) { diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index bc8d46d2984..6fe414c89ef 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -43,8 +43,41 @@ var ( // errUnmintableScope means the scope persisted against a grant names // something no API key can be minted from. errUnmintableScope = xerrors.New("scope is not a valid API key scope") + // errStaleScope means the app's registered scopes narrowed after its + // authorization code was issued and no longer cover the code's scope. + errStaleScope = xerrors.New("scope is no longer allowed by this app's registered scopes") ) +// scopeStillCoveredByAllowlist re-checks the scope a grant was issued with +// against the app's registered scopes as they stand now, since an admin can +// narrow them inside an authorization code's ten minute life. +// +// Refresh deliberately does not call this: RFC 6749 §6 bounds a refresh by the +// scope originally granted, so a narrowing takes effect at the next +// authorization rather than dropping capability from a live session. +func scopeStillCoveredByAllowlist(ctx context.Context, logger slog.Logger, app database.OAuth2ProviderApp, granted string) error { + if noScopeAllowlist(app.Scope) { + return nil + } + + allowlist := grantableScopes(app.Scope.String) + if len(allowlist) == 0 { + // Named verbatim for the same reason as in negotiateScope. + return xerrors.Errorf("%q: %w", app.Scope.String, errNoGrantableScope) + } + + // Canonicalized rather than assumed: the row may have been written by + // another version of this server. + outside, err := firstScopeOutsideAllowlist(ctx, logger, app, allowlist, canonicalScopes(strings.Fields(granted))) + if err != nil { + return err + } + if outside != "" { + return xerrors.Errorf("%q: %w", outside, errStaleScope) + } + return nil +} + // scopeStringToAPIKeyScopes converts the scope persisted on an authorization // code or refresh token into the scope list an API key is minted with. Names // are checked here, not in apikey.Generate, whose error would surface as a 500; @@ -158,7 +191,7 @@ func extractTokenRequest(r *http.Request, callbackURL *url.URL) (codersdk.OAuth2 // Tokens // Uses Sessions.DefaultDuration for access token (API key) TTL and // Sessions.RefreshDefaultDuration for refresh token TTL. -func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerFunc { +func Tokens(db database.Store, lifetimes codersdk.SessionLifetime, logger slog.Logger) http.HandlerFunc { return func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() app := httpmw.OAuth2ProviderApp(r) @@ -220,7 +253,7 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerF case codersdk.OAuth2ProviderGrantTypeRefreshToken: token, err = refreshTokenGrant(ctx, db, app, lifetimes, req) case codersdk.OAuth2ProviderGrantTypeAuthorizationCode: - token, err = authorizationCodeGrant(ctx, db, app, lifetimes, req) + token, err = authorizationCodeGrant(ctx, db, logger, app, lifetimes, req) default: // This should handle truly invalid grant types httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeUnsupportedGrantType, fmt.Sprintf("The grant type %q is not supported", req.GrantType)) @@ -247,9 +280,10 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerF httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidGrant, "The refresh token is invalid or expired") return } - if errors.Is(err, errUnmintableScope) { - // The grant is well-formed and its stored scope is not mintable, so - // a defined OAuth2 failure beats a 500. + // The grant is well-formed and its stored scope cannot be honored, so a + // defined OAuth2 failure beats a 500. + if errors.Is(err, errUnmintableScope) || errors.Is(err, errStaleScope) || + errors.Is(err, errNoGrantableScope) || errors.Is(err, errCoverageUndecidable) { httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidScope, err.Error()) return } @@ -296,7 +330,7 @@ func revokeOAuth2CodeOnPKCEFailure(ctx context.Context, db database.Store, codeI } } -func authorizationCodeGrant(ctx context.Context, db database.Store, app database.OAuth2ProviderApp, lifetimes codersdk.SessionLifetime, req codersdk.OAuth2TokenRequest) (codersdk.OAuth2TokenResponse, error) { +func authorizationCodeGrant(ctx context.Context, db database.Store, logger slog.Logger, app database.OAuth2ProviderApp, lifetimes codersdk.SessionLifetime, req codersdk.OAuth2TokenRequest) (codersdk.OAuth2TokenResponse, error) { // Validate the client secret. secret, err := ParseFormattedSecret(req.ClientSecret) if err != nil { @@ -398,6 +432,10 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database return codersdk.OAuth2TokenResponse{}, errInvalidResource } + if err := scopeStillCoveredByAllowlist(ctx, logger, app, dbCode.Scope); err != nil { + return codersdk.OAuth2TokenResponse{}, err + } + // Generate a refresh token. refreshToken, err := GenerateSecret() if err != nil { diff --git a/coderd/oauth2provider/tokens_internal_test.go b/coderd/oauth2provider/tokens_internal_test.go index ad8f6568eac..3709317e79a 100644 --- a/coderd/oauth2provider/tokens_internal_test.go +++ b/coderd/oauth2provider/tokens_internal_test.go @@ -1,13 +1,17 @@ package oauth2provider import ( + "database/sql" "net/http" "net/url" "strings" "testing" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" @@ -74,6 +78,105 @@ func TestScopeStringToAPIKeyScopes(t *testing.T) { }) } +func TestScopeStillCoveredByAllowlist(t *testing.T) { + t.Parallel() + + const ( + inCatalog = "coder:workspaces.access" + alsoInCatalog = "coder:templates.build" + ) + + tests := []struct { + name string + granted string + appScope sql.NullString + wantErr error + }{ + { + name: "NoAllowlistConstrainsNothing", + granted: string(database.ApiKeyScopeCoderAll), + appScope: sql.NullString{}, + }, + { + name: "EmptyAllowlistConstrainsNothing", + granted: "workspace:ssh", + appScope: sql.NullString{String: "", Valid: true}, + }, + { + name: "UnchangedAllowlistStillCovers", + granted: inCatalog, + appScope: sql.NullString{String: inCatalog, Valid: true}, + }, + { + name: "CompositeStillCoversItsParts", + granted: "workspace:ssh", + appScope: sql.NullString{String: inCatalog, Valid: true}, + }, + { + name: "WidenedAllowlistStillCovers", + granted: "workspace:ssh", + appScope: sql.NullString{String: inCatalog + " " + alsoInCatalog, Valid: true}, + }, + { + name: "AllowlistNarrowedAwayRejected", + granted: "workspace:ssh", + appScope: sql.NullString{String: alsoInCatalog, Valid: true}, + wantErr: errStaleScope, + }, + { + name: "PartiallyCoveredRejectedWhole", + granted: "workspace:ssh file:create", + appScope: sql.NullString{String: inCatalog, Valid: true}, + wantErr: errStaleScope, + }, + { + name: "UnrestrictedGrantNarrowedRejected", + granted: string(database.ApiKeyScopeCoderAll), + appScope: sql.NullString{String: inCatalog, Valid: true}, + wantErr: errStaleScope, + }, + { + name: "AllowlistFilteredToEmptyRejected", + granted: "workspace:ssh", + appScope: sql.NullString{String: "openid profile", Valid: true}, + wantErr: errNoGrantableScope, + }, + { + name: "WhitespaceOnlyAllowlistRejected", + granted: "workspace:ssh", + appScope: sql.NullString{String: " ", Valid: true}, + wantErr: errNoGrantableScope, + }, + { + name: "LegacyAliasAllowlistCoversCanonicalGrant", + granted: "coder:all", + appScope: sql.NullString{String: "all", Valid: true}, + }, + { + name: "GrantOutsideTheCatalogUndecidable", + granted: "some_removed_scope", + appScope: sql.NullString{String: inCatalog, Valid: true}, + wantErr: errCoverageUndecidable, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + app := database.OAuth2ProviderApp{ID: uuid.New(), Scope: test.appScope} + err := scopeStillCoveredByAllowlist(t.Context(), slogtest.Make(t, nil), app, test.granted) + if test.wantErr == nil { + require.NoError(t, err) + return + } + require.ErrorIs(t, err, test.wantErr) + assert.Equal(t, 1, strings.Count(err.Error(), test.wantErr.Error()), + "the rejection reason must appear once, not doubled by the wrap") + }) + } +} + // TestExtractTokenParams_Scopes tests OAuth2 scope parameter parsing // to ensure RFC 6749 compliance where scopes are space-delimited func TestExtractTokenParams_Scopes(t *testing.T) { diff --git a/coderd/oauth2provider/tokens_test.go b/coderd/oauth2provider/tokens_test.go index ca6eafcfa20..33008828876 100644 --- a/coderd/oauth2provider/tokens_test.go +++ b/coderd/oauth2provider/tokens_test.go @@ -159,16 +159,63 @@ func TestOAuth2TokenExchangeScope(t *testing.T) { status, body := postTokenRequest(ctx, t, client, tokenExchangeForm(app, code, verifier)) - require.Equal(t, http.StatusBadRequest, status, body) - var oauthErr struct { - Error string `json:"error"` - ErrorDescription string `json:"error_description"` - } - require.NoError(t, json.Unmarshal([]byte(body), &oauthErr)) - require.Equal(t, string(codersdk.OAuth2ErrorCodeInvalidScope), oauthErr.Error) - require.Contains(t, oauthErr.ErrorDescription, scopeOutOfCatalog, + description := requireTokenScopeError(t, status, body) + require.Contains(t, description, scopeOutOfCatalog, "an operator cannot act on this without knowing which stored name is the problem") }) + + t.Run("AllowlistNarrowedAfterAuthorizationRejected", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + setAppAllowlist(ctx, t, db, app, sql.NullString{String: scopeAlsoInCatalog, Valid: true}) + + status, body := postTokenRequest(ctx, t, client, tokenExchangeForm(app, code, verifier)) + + description := requireTokenScopeError(t, status, body) + require.Contains(t, description, oauth2provider.ReasonStaleScope) + require.Contains(t, description, "workspace:ssh", + "the client needs the scope name to know what was withdrawn") + }) + + t.Run("AllowlistWidenedAfterAuthorizationStillRedeems", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + setAppAllowlist(ctx, t, db, app, sql.NullString{String: scopeInCatalog + " " + scopeAlsoInCatalog, Valid: true}) + + token := exchangeCode(ctx, t, client, app, code, verifier) + + require.Equal(t, database.APIKeyScopes{database.ApiKeyScopeWorkspaceSsh}, + mintedKeyScopes(ctx, t, db, token.RefreshToken)) + }) + + // RFC 6749 §6 bounds a refresh by the scope originally granted, not by the + // live allowlist. + t.Run("RefreshIgnoresAllowlistNarrowing", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true}) + code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "workspace:ssh") + token := exchangeCode(ctx, t, client, app, code, verifier) + setAppAllowlist(ctx, t, db, app, sql.NullString{String: scopeAlsoInCatalog, Valid: true}) + + form := url.Values{} + form.Set("grant_type", "refresh_token") + form.Set("refresh_token", token.RefreshToken) + form.Set("client_id", app.ID.String()) + form.Set("client_secret", app.ClientSecret) + status, body := postTokenRequest(ctx, t, client, form) + refreshed := requireTokenResponse(t, status, body) + + require.Equal(t, database.APIKeyScopes{database.ApiKeyScopeWorkspaceSsh}, + mintedKeyScopes(ctx, t, db, refreshed.RefreshToken)) + }) } // appWithSecret is seeded directly because the management API registers no @@ -203,6 +250,38 @@ func seedAppWithSecret(t *testing.T, db database.Store, allowlist sql.NullString } } +// setAppAllowlist rewrites an app's registered scopes, leaving every other +// column as seeded. +func setAppAllowlist(ctx context.Context, t *testing.T, db database.Store, app appWithSecret, allowlist sql.NullString) { + t.Helper() + + _, err := db.UpdateOAuth2ProviderAppByID(dbauthz.AsSystemRestricted(ctx), database.UpdateOAuth2ProviderAppByIDParams{ + ID: app.ID, + UpdatedAt: dbtime.Now(), + Name: app.Name, + Icon: app.Icon, + CallbackURL: app.CallbackURL, + RedirectUris: app.RedirectUris, + ClientType: app.ClientType, + DynamicallyRegistered: app.DynamicallyRegistered, + ClientSecretExpiresAt: app.ClientSecretExpiresAt, + GrantTypes: app.GrantTypes, + ResponseTypes: app.ResponseTypes, + TokenEndpointAuthMethod: app.TokenEndpointAuthMethod, + Scope: allowlist, + Contacts: app.Contacts, + ClientUri: app.ClientUri, + LogoUri: app.LogoUri, + TosUri: app.TosUri, + PolicyUri: app.PolicyUri, + JwksUri: app.JwksUri, + Jwks: app.Jwks, + SoftwareID: app.SoftwareID, + SoftwareVersion: app.SoftwareVersion, + }) + require.NoError(t, err) +} + // Returns the secret that redeems the row. dbgen is unusable here: it derives // expires_at from created_at, leaving the row already expired. func seedRefreshToken(ctx context.Context, t *testing.T, db database.Store, app appWithSecret, userID uuid.UUID, scope string) string { @@ -318,6 +397,21 @@ func requireTokenResponse(t *testing.T, status int, body string) codersdk.OAuth2 return token } +// requireTokenScopeError asserts an RFC 6749 §5.2 invalid_scope response and +// returns its description. +func requireTokenScopeError(t *testing.T, status int, body string) string { + t.Helper() + + require.Equal(t, http.StatusBadRequest, status, body) + var oauthErr struct { + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + } + require.NoError(t, json.Unmarshal([]byte(body), &oauthErr)) + require.Equal(t, string(codersdk.OAuth2ErrorCodeInvalidScope), oauthErr.Error) + return oauthErr.ErrorDescription +} + func mintedKeyScopes(ctx context.Context, t *testing.T, db database.Store, refreshToken string) database.APIKeyScopes { t.Helper() diff --git a/docs/admin/integrations/oauth2-provider.md b/docs/admin/integrations/oauth2-provider.md index c83885bbb00..c886fef87c0 100644 --- a/docs/admin/integrations/oauth2-provider.md +++ b/docs/admin/integrations/oauth2-provider.md @@ -407,6 +407,18 @@ scope names something this deployment cannot mint, the exchange answers HTTP The usual cause is a grant made against a scope the deployment has since dropped. Authorize again to negotiate a scope it still supports. +The exchange also re-checks the code's scope against the application's +registered `scope`, which an administrator can narrow during the ten minutes a +code stays valid. A code whose scope the narrowed registration no longer +covers is refused the same way, with `scope is no longer allowed by this app's +registered scopes`. Authorize again to negotiate a scope within the new +registration. + +A refresh is not re-checked against the registration. RFC 6749 section 6 bounds +it by the scope originally granted, so narrowing an application's registered +scopes takes effect at the next authorization rather than cutting short a +session already in progress. + ### "PKCE verification failed" Verify that the `code_verifier` used in the token request matches the one used to generate the `code_challenge`.