From b6f74c38deeef126d0a8252eda8f0fe1ec643a0a Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Fri, 28 Aug 2026 00:50:54 +0000 Subject: [PATCH 1/2] feat: re-check the app allowlist at OAuth2 code redemption An authorization code's scope is checked against the app's registered scopes when the code is issued, and the code stays valid for ten minutes after that. An admin who narrows the registration in that window had the narrowing ignored: the code still minted a token carrying the wider scope. authorizationCodeGrant now re-checks the code's stored scope against the registration as it stands at redemption, and refuses with invalid_scope when it is no longer covered. The catalog filter and the coverage loop move out of negotiateScope into grantableScopes and firstScopeOutsideAllowlist so both sides run the same comparison; each caller picks its own rejection reason. Refresh is deliberately left alone. RFC 6749 section 6 bounds a refresh by the scope originally granted, so a narrowing takes effect at the next authorization instead of dropping capability from a live session. --- coderd/oauth2.go | 2 +- coderd/oauth2provider/authorize.go | 97 +++++++++------ coderd/oauth2provider/tokens.go | 59 ++++++++- coderd/oauth2provider/tokens_internal_test.go | 113 +++++++++++++++++ coderd/oauth2provider/tokens_test.go | 116 ++++++++++++++++-- docs/admin/integrations/oauth2-provider.md | 12 ++ 6 files changed, 347 insertions(+), 52 deletions(-) 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..b999ea3555e 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,56 @@ func noScopeAllowlist(appScope sql.NullString) bool { return !appScope.Valid || appScope.String == "" } +// grantableScopes narrows an app's registered allowlist to the names this +// deployment offers, canonicalized and deduplicated. The stored allowlist may +// name a scope since removed from the catalog, or never in it; dropping those +// only ever narrows what can be granted. An empty result means the allowlist +// grants nothing at all, which negotiation and redemption report differently, +// so it is returned rather than rejected here. +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 whose +// permissions the allowlist does not confer, or "" when it confers all of them. +// 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` +// covers `workspace:read`. Both arguments must already be canonical. +// +// A comparison RBAC cannot answer refuses rather than grants, returning +// errCoverageUndecidable. The underlying error names RBAC internals, so it goes +// to the log rather than to the client. +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 +154,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/tokens.go b/coderd/oauth2provider/tokens.go index bc8d46d2984..924e76d498e 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -43,8 +43,47 @@ 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 no longer cover the scope + // its authorization code was issued with. + 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. An admin can narrow +// them inside an authorization code's ten minute life, and the code's scope was +// last checked when it was issued. +// +// An app with no registered scopes constrains nothing, so nothing is re-checked. +// An app that has since gained them is checked against them, since that +// narrowing is the case this exists for. +// +// Refresh deliberately does not call this: RFC 6749 §6 bounds a refresh by the +// scope originally granted, so an allowlist narrowing takes effect at the next +// authorization rather than silently 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 { + // An allowlist that grants nothing covers nothing. Named verbatim for + // the same reason as in negotiateScope. + return xerrors.Errorf("%q: %w", app.Scope.String, errNoGrantableScope) + } + + // Canonicalized rather than assumed: negotiateScope writes canonical names, + // but 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 +197,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 +259,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 +286,11 @@ 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: it is + // not mintable, or the app's registered scopes have narrowed under it. 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 +337,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 +439,12 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database return codersdk.OAuth2TokenResponse{}, errInvalidResource } + // The scope was checked against the allowlist when the code was issued, and + // an admin can have narrowed it since. + 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..9a8e8a25ce9 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,115 @@ 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}, + }, + { + // The control for the case below: an edit that leaves the grant + // covered must not reject it. + 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, + }, + { + // An app with no allowlist grants coder:all. Adding one narrows it, + // which is the same narrowing seen from the other side. + 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}, + }, + { + // Refused rather than granted: RBAC cannot expand the stored name, + // so coverage has no answer. + 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") + }) + } +} + +// Rejection reason for the package's black-box tests, which cannot reach the +// sentinel. +var ReasonStaleScope = errStaleScope.Error() + // 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..9d29a0d2fd8 100644 --- a/coderd/oauth2provider/tokens_test.go +++ b/coderd/oauth2provider/tokens_test.go @@ -159,16 +159,68 @@ 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") }) + + // A code lives for ten minutes, and its scope was last checked against the + // allowlist when it was issued. + 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 cannot tell which of its scopes was withdrawn without the name") + }) + + // The control for the case above. An allowlist edit that still covers the + // grant must leave the code redeemable, and must not widen what it mints. + 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. An admin narrowing it takes effect at the next + // authorization rather than dropping capability from a session mid-use. + 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 +255,38 @@ func seedAppWithSecret(t *testing.T, db database.Store, allowlist sql.NullString } } +// setAppAllowlist rewrites an app's registered scopes the way an admin edit +// would, 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 +402,22 @@ 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, which is what tells a client or operator which +// scope was at fault. +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`. From 9f3193e29b603f9934d10ff2069d7880a613041a Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Fri, 28 Aug 2026 01:33:40 +0000 Subject: [PATCH 2/2] docs(coderd/oauth2provider): trim the allowlist re-check comments Drop what the code already says and keep the parts that are not visible from it: the canonical precondition on firstScopeOutsideAllowlist, why refresh is exempt, and why the stored scope is canonicalized again. ReasonStaleScope moves into the existing export block in authorize_internal_test.go rather than repeating its comment. --- coderd/oauth2provider/authorize.go | 24 ++++++--------- .../oauth2provider/authorize_internal_test.go | 1 + coderd/oauth2provider/tokens.go | 29 +++++++------------ coderd/oauth2provider/tokens_internal_test.go | 10 ------- coderd/oauth2provider/tokens_test.go | 16 ++++------ 5 files changed, 25 insertions(+), 55 deletions(-) diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index b999ea3555e..36e09778612 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -70,12 +70,10 @@ func noScopeAllowlist(appScope sql.NullString) bool { return !appScope.Valid || appScope.String == "" } -// grantableScopes narrows an app's registered allowlist to the names this -// deployment offers, canonicalized and deduplicated. The stored allowlist may -// name a scope since removed from the catalog, or never in it; dropping those -// only ever narrows what can be granted. An empty result means the allowlist -// grants nothing at all, which negotiation and redemption report differently, -// so it is returned rather than rejected here. +// 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)) @@ -89,15 +87,11 @@ func grantableScopes(appScope string) []string { return canonicalScopes(filtered) } -// firstScopeOutsideAllowlist returns the first scope in granted whose -// permissions the allowlist does not confer, or "" when it confers all of them. -// 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` -// covers `workspace:read`. Both arguments must already be canonical. -// -// A comparison RBAC cannot answer refuses rather than grants, returning -// errCoverageUndecidable. The underlying error names RBAC internals, so it goes -// to the log rather than to the client. +// 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 { 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 924e76d498e..6fe414c89ef 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -43,23 +43,18 @@ 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 no longer cover the scope - // its authorization code was issued with. + // 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. An admin can narrow -// them inside an authorization code's ten minute life, and the code's scope was -// last checked when it was issued. -// -// An app with no registered scopes constrains nothing, so nothing is re-checked. -// An app that has since gained them is checked against them, since that -// narrowing is the case this exists for. +// 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 an allowlist narrowing takes effect at the next -// authorization rather than silently dropping capability from a live session. +// 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 @@ -67,13 +62,12 @@ func scopeStillCoveredByAllowlist(ctx context.Context, logger slog.Logger, app d allowlist := grantableScopes(app.Scope.String) if len(allowlist) == 0 { - // An allowlist that grants nothing covers nothing. Named verbatim for - // the same reason as in negotiateScope. + // Named verbatim for the same reason as in negotiateScope. return xerrors.Errorf("%q: %w", app.Scope.String, errNoGrantableScope) } - // Canonicalized rather than assumed: negotiateScope writes canonical names, - // but the row may have been written by another version of this server. + // 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 @@ -286,8 +280,7 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime, logger slog.L httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidGrant, "The refresh token is invalid or expired") return } - // The grant is well-formed and its stored scope cannot be honored: it is - // not mintable, or the app's registered scopes have narrowed under it. A + // 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) { @@ -439,8 +432,6 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, logger slog. return codersdk.OAuth2TokenResponse{}, errInvalidResource } - // The scope was checked against the allowlist when the code was issued, and - // an admin can have narrowed it since. if err := scopeStillCoveredByAllowlist(ctx, logger, app, dbCode.Scope); err != nil { return codersdk.OAuth2TokenResponse{}, err } diff --git a/coderd/oauth2provider/tokens_internal_test.go b/coderd/oauth2provider/tokens_internal_test.go index 9a8e8a25ce9..3709317e79a 100644 --- a/coderd/oauth2provider/tokens_internal_test.go +++ b/coderd/oauth2provider/tokens_internal_test.go @@ -113,8 +113,6 @@ func TestScopeStillCoveredByAllowlist(t *testing.T) { appScope: sql.NullString{String: inCatalog, Valid: true}, }, { - // The control for the case below: an edit that leaves the grant - // covered must not reject it. name: "WidenedAllowlistStillCovers", granted: "workspace:ssh", appScope: sql.NullString{String: inCatalog + " " + alsoInCatalog, Valid: true}, @@ -132,8 +130,6 @@ func TestScopeStillCoveredByAllowlist(t *testing.T) { wantErr: errStaleScope, }, { - // An app with no allowlist grants coder:all. Adding one narrows it, - // which is the same narrowing seen from the other side. name: "UnrestrictedGrantNarrowedRejected", granted: string(database.ApiKeyScopeCoderAll), appScope: sql.NullString{String: inCatalog, Valid: true}, @@ -157,8 +153,6 @@ func TestScopeStillCoveredByAllowlist(t *testing.T) { appScope: sql.NullString{String: "all", Valid: true}, }, { - // Refused rather than granted: RBAC cannot expand the stored name, - // so coverage has no answer. name: "GrantOutsideTheCatalogUndecidable", granted: "some_removed_scope", appScope: sql.NullString{String: inCatalog, Valid: true}, @@ -183,10 +177,6 @@ func TestScopeStillCoveredByAllowlist(t *testing.T) { } } -// Rejection reason for the package's black-box tests, which cannot reach the -// sentinel. -var ReasonStaleScope = errStaleScope.Error() - // 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 9d29a0d2fd8..33008828876 100644 --- a/coderd/oauth2provider/tokens_test.go +++ b/coderd/oauth2provider/tokens_test.go @@ -164,8 +164,6 @@ func TestOAuth2TokenExchangeScope(t *testing.T) { "an operator cannot act on this without knowing which stored name is the problem") }) - // A code lives for ten minutes, and its scope was last checked against the - // allowlist when it was issued. t.Run("AllowlistNarrowedAfterAuthorizationRejected", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) @@ -179,11 +177,9 @@ func TestOAuth2TokenExchangeScope(t *testing.T) { description := requireTokenScopeError(t, status, body) require.Contains(t, description, oauth2provider.ReasonStaleScope) require.Contains(t, description, "workspace:ssh", - "the client cannot tell which of its scopes was withdrawn without the name") + "the client needs the scope name to know what was withdrawn") }) - // The control for the case above. An allowlist edit that still covers the - // grant must leave the code redeemable, and must not widen what it mints. t.Run("AllowlistWidenedAfterAuthorizationStillRedeems", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) @@ -199,8 +195,7 @@ func TestOAuth2TokenExchangeScope(t *testing.T) { }) // RFC 6749 §6 bounds a refresh by the scope originally granted, not by the - // live allowlist. An admin narrowing it takes effect at the next - // authorization rather than dropping capability from a session mid-use. + // live allowlist. t.Run("RefreshIgnoresAllowlistNarrowing", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) @@ -255,8 +250,8 @@ func seedAppWithSecret(t *testing.T, db database.Store, allowlist sql.NullString } } -// setAppAllowlist rewrites an app's registered scopes the way an admin edit -// would, leaving every other column as seeded. +// 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() @@ -403,8 +398,7 @@ func requireTokenResponse(t *testing.T, status int, body string) codersdk.OAuth2 } // requireTokenScopeError asserts an RFC 6749 §5.2 invalid_scope response and -// returns its description, which is what tells a client or operator which -// scope was at fault. +// returns its description. func requireTokenScopeError(t *testing.T, status int, body string) string { t.Helper()