mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
Layer 1 of a multi-PR split of #27195 (public/secretless PKCE-only OAuth2 clients), broken up for easier review: **database schema (this PR)** → oauth2provider handler logic → API/e2e integration tests. ## Goal Coder's OAuth2 provider only works correctly for confidential clients today. Public clients — native apps that can't safely hold a shared secret, such as the CLI's browser-based login flow, IDE plugins (VS Code, JetBrains), desktop apps, and MCP clients — cannot complete a real OAuth2 flow against Coder, even though OAuth 2.1 §2.1 explicitly defines this client type and RFC 8252 §8.5 requires PKCE alone to be sufficient authentication for it. Every MCP client, CLI login flow, and IDE plugin is a public client by construction, and none of them can complete a secretless flow against Coder today: dynamic registration always classifies a client as confidential regardless of what it asks for, the token endpoint unconditionally requires a `client_secret`, and discovery metadata never advertises `"none"` as a supported auth method. Full write-up: [ENG-3029](https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client) ### Overall design (end state across the full PR stack) `[PR2]` marks handler-layer changes landing in the next PR in this stack. The green box is what this PR implements. ```mermaid sequenceDiagram autonumber participant C as Public Client (CLI/MCP/IDE plugin) participant S as coderd (chi router) participant H as oauth2provider handlers participant DB as PostgreSQL Note over C,S: Discovery C->>S: GET /.well-known/oauth-authorization-server S->>H: GetAuthorizationServerMetadata() Note over H: [PR2] add "none" to<br/>the returned auth methods list H-->>C: [PR2] 200 { token_endpoint_auth_methods_supported:<br/>[..., "none"] } Note over C,S: Dynamic Client Registration C->>S: POST /oauth2/register<br/>{redirect_uris, token_endpoint_auth_method: "none"} S->>H: CreateDynamicClientRegistration() Note over H: [PR2] client type now reads<br/>the request -> "public" Note over H: [PR2] skip secret generation<br/>for public clients H->>DB: [PR2] INSERT app row<br/>(client_type = 'public') DB-->>H: app row Note over H: [PR2] skip secret insert entirely H-->>C: [PR2] 201 { client_id }<br/>(no client_secret field) Note over C,S: Authorization Code + PKCE flow C->>S: GET /oauth2/authorize?client_id=...&code_challenge=... C->>S: POST /oauth2/tokens (grant_type=authorization_code)<br/>no client_secret S->>H: extractTokenRequest() Note over H: [PR2] client_secret no longer required<br/>for public clients H->>H: authorizationCodeGrant() Note over H: [PR2] skip secret lookup for public clients Note over H: PKCE verification — already mandatory, unchanged rect rgb(198, 239, 206) Note over H,DB: [THIS PR] oauth2_provider_app_tokens.app_id<br/>column added (NOT NULL, populated at insert<br/>time from app.ID) and app_secret_id loosened<br/>to nullable. Revocation now checks app_id<br/>directly. Confidential-client behavior is<br/>unchanged — no public client can be created yet. H->>DB: [PR2] INSERT refresh token row<br/>(no secret reference, for public clients) end DB-->>H: token row H-->>C: 200 { access_token, refresh_token } ``` ## This PR: database schema A public client has no `client_secret`, so it has nothing to put in `oauth2_provider_app_tokens.app_secret_id`, which was `NOT NULL`. This PR makes that column nullable and instead attributes a token to its owning app through a new, always-populated `app_id` column — so ownership checks (e.g. revocation) work identically for public and confidential clients without joining through a secret that may not exist. | Column | Before | After (this PR) | |---|---|---| | `app_secret_id` | `uuid NOT NULL` | **nullable** | | `app_id` | — | **new**: `uuid NOT NULL`, `FOREIGN KEY → oauth2_provider_apps(id) ON DELETE CASCADE`, backfilled for every existing row and populated on every new insert from that point on | This is a single, complete migration — not staged across multiple PRs. An earlier version of this branch deferred `app_secret_id`'s nullability and the insert-time population of `app_id` to a later PR, keeping this PR's diff limited to `coderd/database`. [Automated review](https://github.com/coder/coder/pull/27712#discussion_r3686851911) correctly flagged that as unsafe: the migration would backfill existing rows once, but nothing would populate `app_id` for rows written afterward, so the moment this PR merged, new tokens would start accumulating a permanently `NULL` app_id — and if a release happened to be cut before the follow-up PR landed, that gap could ship to customers and would need a second, later backfill to close. Doing the full migration now avoids that: `app_id` is correct from the first row written, and the promised `NOT NULL` constraint requires no data repair because it's already enforced. Closing that gap requires a few mechanical, non-branching touches outside `coderd/database`: - `revoke.go`'s two ownership checks now compare `dbToken.AppID` directly instead of looking up the app through `app_secret_id` — a genuine simplification (and slightly less code), not a temporary shim. - `tokens.go`'s two `InsertOAuth2ProviderAppToken` call sites supply the new `app_id` column and wrap `app_secret_id` as a `NullUUID`. - `oauth2_test.go`'s one direct-insert test fixture does the same. None of these introduce client-type branching or new capability — every client today is still confidential-only, still always presents a secret, and behavior is unchanged. The full repo builds, vets, and all existing tests pass unmodified in behavior. ## Coming next - **PR2 (handler layer)**: `codersdk`'s `DetermineClientType()` reading the requested `token_endpoint_auth_method`; `registration.go` skipping secret generation for public clients (and wrapping the app+secret insert in a single transaction, fixing a pre-existing orphan-row/visibility-race gap); `tokens.go` making the secret check conditional so PKCE alone authenticates a public client; `metadata.go` advertising `"none"` in discovery. No further migration is needed — the schema this PR ships is already final. - **PR3 (API/e2e layer)**: integration tests through the real HTTP API (`coderd/oauth2_test.go`), the MCP OAuth2 e2e flow (`coderd/mcp/mcp_e2e_test.go`), and the manual test script (`scripts/oauth2/test-mcp-oauth2.sh`). Depends on: #27195 (original combined PR, being superseded by this stack) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
246 lines
8.5 KiB
Go
246 lines
8.5 KiB
Go
package oauth2provider
|
|
|
|
import (
|
|
"context"
|
|
"crypto/sha256"
|
|
"crypto/subtle"
|
|
"database/sql"
|
|
"errors"
|
|
"net/http"
|
|
"strings"
|
|
|
|
"github.com/google/uuid"
|
|
"golang.org/x/xerrors"
|
|
|
|
"cdr.dev/slog/v3"
|
|
"github.com/coder/coder/v2/coderd/apikey"
|
|
"github.com/coder/coder/v2/coderd/database"
|
|
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
|
"github.com/coder/coder/v2/coderd/httpapi"
|
|
"github.com/coder/coder/v2/coderd/httpmw"
|
|
"github.com/coder/coder/v2/codersdk"
|
|
)
|
|
|
|
var (
|
|
// ErrTokenNotBelongsToClient is returned when a token does not belong to the requesting client
|
|
ErrTokenNotBelongsToClient = xerrors.New("token does not belong to requesting client")
|
|
// ErrInvalidTokenFormat is returned when a token has an invalid format
|
|
ErrInvalidTokenFormat = xerrors.New("invalid token format")
|
|
)
|
|
|
|
func extractRevocationRequest(r *http.Request) (codersdk.OAuth2TokenRevocationRequest, error) {
|
|
if err := r.ParseForm(); err != nil {
|
|
return codersdk.OAuth2TokenRevocationRequest{}, xerrors.Errorf("invalid form data: %w", err)
|
|
}
|
|
|
|
req := codersdk.OAuth2TokenRevocationRequest{
|
|
Token: r.Form.Get("token"),
|
|
TokenTypeHint: codersdk.OAuth2RevocationTokenTypeHint(r.Form.Get("token_type_hint")),
|
|
ClientID: r.Form.Get("client_id"),
|
|
ClientSecret: r.Form.Get("client_secret"),
|
|
}
|
|
|
|
// RFC 7009 requires 'token' parameter.
|
|
if req.Token == "" {
|
|
return codersdk.OAuth2TokenRevocationRequest{}, xerrors.New("missing token parameter")
|
|
}
|
|
|
|
return req, nil
|
|
}
|
|
|
|
// RevokeToken implements RFC 7009 OAuth2 Token Revocation
|
|
// Authentication is unique for this endpoint in that it does not use the
|
|
// standard token authentication middleware. Instead, it expects the token that
|
|
// is being revoked to be valid.
|
|
// TODO: Currently the token validation occurs in the revocation logic itself.
|
|
// This code should be refactored to share token validation logic with other parts
|
|
// of the OAuth2 provider/http middleware.
|
|
func RevokeToken(db database.Store, logger slog.Logger) http.HandlerFunc {
|
|
return func(rw http.ResponseWriter, r *http.Request) {
|
|
ctx := r.Context()
|
|
app := httpmw.OAuth2ProviderApp(r)
|
|
|
|
// RFC 7009 requires POST method with application/x-www-form-urlencoded
|
|
if r.Method != http.MethodPost {
|
|
httpapi.WriteOAuth2Error(ctx, rw, http.StatusMethodNotAllowed, codersdk.OAuth2ErrorCodeInvalidRequest, "Method not allowed")
|
|
return
|
|
}
|
|
|
|
req, err := extractRevocationRequest(r)
|
|
if err != nil {
|
|
httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error())
|
|
return
|
|
}
|
|
|
|
// Determine if this is a refresh token (starts with "coder_") or API key
|
|
// APIKeys do not have the SecretIdentifier prefix.
|
|
const coderPrefix = SecretIdentifier + "_"
|
|
isRefreshToken := strings.HasPrefix(req.Token, coderPrefix)
|
|
|
|
// Revoke the token with ownership verification
|
|
err = db.InTx(func(tx database.Store) error {
|
|
if isRefreshToken {
|
|
// Handle refresh token revocation
|
|
return revokeRefreshTokenInTx(ctx, tx, req.Token, app.ID)
|
|
}
|
|
// Handle API key revocation
|
|
return revokeAPIKeyInTx(ctx, tx, req.Token, app.ID)
|
|
}, nil)
|
|
if err != nil {
|
|
if errors.Is(err, ErrTokenNotBelongsToClient) {
|
|
// RFC 7009: Return success even if token doesn't belong to client (don't reveal token existence)
|
|
logger.Debug(ctx, "token revocation failed: token does not belong to requesting client",
|
|
slog.F("client_id", app.ID.String()),
|
|
slog.F("app_name", app.Name))
|
|
rw.WriteHeader(http.StatusOK)
|
|
return
|
|
}
|
|
if errors.Is(err, ErrInvalidTokenFormat) {
|
|
// Invalid token format should return 400 bad request
|
|
logger.Debug(ctx, "token revocation failed: invalid token format",
|
|
slog.F("client_id", app.ID.String()),
|
|
slog.F("app_name", app.Name))
|
|
httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, "Invalid token format")
|
|
return
|
|
}
|
|
logger.Error(ctx, "token revocation failed with internal server error",
|
|
slog.Error(err),
|
|
slog.F("client_id", app.ID.String()),
|
|
slog.F("app_name", app.Name))
|
|
httpapi.WriteOAuth2Error(ctx, rw, http.StatusInternalServerError, codersdk.OAuth2ErrorCodeServerError, "Internal server error")
|
|
return
|
|
}
|
|
|
|
// RFC 7009: successful revocation returns HTTP 200
|
|
rw.WriteHeader(http.StatusOK)
|
|
}
|
|
}
|
|
|
|
func revokeRefreshTokenInTx(ctx context.Context, db database.Store, token string, appID uuid.UUID) error {
|
|
// Parse the refresh token using the existing function
|
|
parsedToken, err := ParseFormattedSecret(token)
|
|
if err != nil {
|
|
return ErrInvalidTokenFormat
|
|
}
|
|
|
|
// Try to find refresh token by prefix
|
|
//nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint
|
|
dbToken, err := db.GetOAuth2ProviderAppTokenByPrefix(dbauthz.AsSystemOAuth2(ctx), []byte(parsedToken.Prefix))
|
|
if err != nil {
|
|
if errors.Is(err, sql.ErrNoRows) {
|
|
// Token not found - return success per RFC 7009 (don't reveal token existence)
|
|
return nil
|
|
}
|
|
return xerrors.Errorf("get oauth2 provider app token by prefix: %w", err)
|
|
}
|
|
|
|
equal := apikey.ValidateHash(dbToken.RefreshHash, parsedToken.Secret)
|
|
if !equal {
|
|
return xerrors.Errorf("invalid refresh token")
|
|
}
|
|
|
|
// Verify ownership directly via app_id, avoiding a join through
|
|
// app_secret_id, which is not always present.
|
|
if dbToken.AppID != appID {
|
|
return ErrTokenNotBelongsToClient
|
|
}
|
|
|
|
// Delete the associated API key, which should cascade to remove the refresh token
|
|
// According to RFC 7009, when a refresh token is revoked, associated access tokens should be invalidated
|
|
//nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint
|
|
err = db.DeleteAPIKeyByID(dbauthz.AsSystemOAuth2(ctx), dbToken.APIKeyID)
|
|
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
|
return xerrors.Errorf("delete api key: %w", err)
|
|
}
|
|
|
|
return nil
|
|
}
|
|
|
|
func revokeAPIKeyInTx(ctx context.Context, db database.Store, token string, appID uuid.UUID) error {
|
|
keyID, secret, err := httpmw.SplitAPIToken(token)
|
|
if err != nil {
|
|
return ErrInvalidTokenFormat
|
|
}
|
|
|
|
// Get the API key
|
|
//nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint
|
|
apiKey, err := db.GetAPIKeyByID(dbauthz.AsSystemOAuth2(ctx), keyID)
|
|
if err != nil {
|
|
if errors.Is(err, sql.ErrNoRows) {
|
|
// API key not found - return success per RFC 7009 (don't reveal token existence)
|
|
return nil
|
|
}
|
|
return xerrors.Errorf("get api key by id: %w", err)
|
|
}
|
|
|
|
// Checking to see if the provided secret matches the stored hashed secret
|
|
hashedSecret := sha256.Sum256([]byte(secret))
|
|
if subtle.ConstantTimeCompare(apiKey.HashedSecret, hashedSecret[:]) != 1 {
|
|
return xerrors.Errorf("invalid api key")
|
|
}
|
|
|
|
// Verify the API key was created by OAuth2
|
|
if apiKey.LoginType != database.LoginTypeOAuth2ProviderApp {
|
|
return xerrors.New("api key is not an oauth2 token")
|
|
}
|
|
|
|
// Find the associated OAuth2 token to verify ownership
|
|
//nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint
|
|
dbToken, err := db.GetOAuth2ProviderAppTokenByAPIKeyID(dbauthz.AsSystemOAuth2(ctx), apiKey.ID)
|
|
if err != nil {
|
|
if errors.Is(err, sql.ErrNoRows) {
|
|
// No associated OAuth2 token - return success per RFC 7009
|
|
return nil
|
|
}
|
|
return xerrors.Errorf("get oauth2 provider app token by api key id: %w", err)
|
|
}
|
|
|
|
// Verify the token belongs to the requesting app directly via app_id,
|
|
// avoiding a join through app_secret_id, which is not always present.
|
|
if dbToken.AppID != appID {
|
|
return ErrTokenNotBelongsToClient
|
|
}
|
|
|
|
// Delete the API key
|
|
//nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint
|
|
err = db.DeleteAPIKeyByID(dbauthz.AsSystemOAuth2(ctx), apiKey.ID)
|
|
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
|
return xerrors.Errorf("delete api key for revocation: %w", err)
|
|
}
|
|
|
|
return nil
|
|
}
|
|
|
|
func RevokeApp(db database.Store) http.HandlerFunc {
|
|
return func(rw http.ResponseWriter, r *http.Request) {
|
|
ctx := r.Context()
|
|
apiKey := httpmw.APIKey(r)
|
|
app := httpmw.OAuth2ProviderApp(r)
|
|
|
|
err := db.InTx(func(tx database.Store) error {
|
|
err := tx.DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx, database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams{
|
|
AppID: app.ID,
|
|
UserID: apiKey.UserID,
|
|
})
|
|
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
|
return err
|
|
}
|
|
|
|
err = tx.DeleteOAuth2ProviderAppTokensByAppAndUserID(ctx, database.DeleteOAuth2ProviderAppTokensByAppAndUserIDParams{
|
|
AppID: app.ID,
|
|
UserID: apiKey.UserID,
|
|
})
|
|
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
|
return err
|
|
}
|
|
|
|
return nil
|
|
}, nil)
|
|
if err != nil {
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
rw.WriteHeader(http.StatusNoContent)
|
|
}
|
|
}
|