Skip to content

Commit fb90065

Browse files
ThomasK33ThomasK33
committed
feat(oauth2): add RFC 8707 resource indicators and audience validation
Implements RFC 8707 Resource Indicators for OAuth2 provider to enable proper audience validation and token binding for multi-tenant scenarios. Key changes: - Add resource parameter support to authorization and token endpoints - Implement server-side audience validation for opaque tokens - Add database fields: ResourceUri (codes) and Audience (tokens) - Add comprehensive resource parameter validation logic - Add cross-resource audience validation in API middleware - Add extensive test coverage for RFC 8707 scenarios - Enhance PKCE implementation with timing attack protection This enables OAuth2 clients to specify target resource servers and prevents token abuse across different Coder deployments through proper audience binding. Change-Id: I3924cb2139e837e3ac0b0bd40a5aeb59637ebc1b Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent 018694a commit fb90065

File tree

12 files changed

+551
-14
lines changed

12 files changed

+551
-14
lines changed

‎CLAUDE.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,29 @@ Read [cursor rules](.cursorrules).
120120
4. Run `make gen` again
121121
5. Run `make lint` to catch any remaining issues
122122

123+
### In-Memory Database Testing
124+
125+
When adding new database fields:
126+
127+
- **CRITICAL**: Update `coderd/database/dbmem/dbmem.go` in-memory implementations
128+
- The `Insert*` functions must include ALL new fields, not just basic ones
129+
- Common issue: Tests pass with real database but fail with in-memory database due to missing field mappings
130+
- Always verify in-memory database functions match the real database schema after migrations
131+
132+
Example pattern:
133+
134+
```go
135+
// In dbmem.go - ensure ALL fields are included
136+
code := database.OAuth2ProviderAppCode{
137+
ID: arg.ID,
138+
CreatedAt: arg.CreatedAt,
139+
// ... existing fields ...
140+
ResourceUri: arg.ResourceUri, // New field
141+
CodeChallenge: arg.CodeChallenge, // New field
142+
CodeChallengeMethod: arg.CodeChallengeMethod, // New field
143+
}
144+
```
145+
123146
## Architecture
124147

125148
### Core Components
@@ -204,6 +227,12 @@ When working on OAuth2 provider features:
204227
- Avoid dependency on referer headers for security decisions
205228
- Support proper state parameter validation
206229

230+
6. **RFC 8707 Resource Indicators**:
231+
- Store resource parameters in database for server-side validation (opaque tokens)
232+
- Validate resource consistency between authorization and token requests
233+
- Support audience validation in refresh token flows
234+
- Resource parameter is optional but must be consistent when provided
235+
207236
### OAuth2 Error Handling Pattern
208237

209238
```go
@@ -258,3 +287,6 @@ Always run the full test suite after OAuth2 changes:
258287
2. **"package should be X_test"** - Use `package_test` naming for test files
259288
3. **SQL type errors** - Use `sql.Null*` types for nullable fields
260289
4. **Missing newlines** - Ensure files end with newline character
290+
5. **OAuth2 tests failing but scripts working** - Check in-memory database implementations in `dbmem.go`
291+
6. **Resource indicator validation failing** - Ensure database stores and retrieves resource parameters correctly
292+
7. **PKCE tests failing** - Verify both authorization code storage and token exchange handle PKCE fields

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2165,6 +2165,25 @@ func (q *querier) GetOAuth2ProviderAppSecretsByAppID(ctx context.Context, appID
21652165
return q.db.GetOAuth2ProviderAppSecretsByAppID(ctx, appID)
21662166
}
21672167

2168+
func (q *querier) GetOAuth2ProviderAppTokenByAPIKeyID(ctx context.Context, apiKeyID string) (database.OAuth2ProviderAppToken, error) {
2169+
token, err := q.db.GetOAuth2ProviderAppTokenByAPIKeyID(ctx, apiKeyID)
2170+
if err != nil {
2171+
return database.OAuth2ProviderAppToken{}, err
2172+
}
2173+
2174+
// Get the associated API key to check ownership
2175+
apiKey, err := q.db.GetAPIKeyByID(ctx, token.APIKeyID)
2176+
if err != nil {
2177+
return database.OAuth2ProviderAppToken{}, err
2178+
}
2179+
2180+
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOauth2AppCodeToken.WithOwner(apiKey.UserID.String())); err != nil {
2181+
return database.OAuth2ProviderAppToken{}, err
2182+
}
2183+
2184+
return token, nil
2185+
}
2186+
21682187
func (q *querier) GetOAuth2ProviderAppTokenByPrefix(ctx context.Context, hashPrefix []byte) (database.OAuth2ProviderAppToken, error) {
21692188
token, err := q.db.GetOAuth2ProviderAppTokenByPrefix(ctx, hashPrefix)
21702189
if err != nil {

‎coderd/database/dbmem/dbmem.go

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4050,6 +4050,19 @@ func (q *FakeQuerier) GetOAuth2ProviderAppSecretsByAppID(_ context.Context, appI
40504050
return []database.OAuth2ProviderAppSecret{}, sql.ErrNoRows
40514051
}
40524052

4053+
func (q *FakeQuerier) GetOAuth2ProviderAppTokenByAPIKeyID(_ context.Context, apiKeyID string) (database.OAuth2ProviderAppToken, error) {
4054+
q.mutex.Lock()
4055+
defer q.mutex.Unlock()
4056+
4057+
for _, token := range q.oauth2ProviderAppTokens {
4058+
if token.APIKeyID == apiKeyID {
4059+
return token, nil
4060+
}
4061+
}
4062+
4063+
return database.OAuth2ProviderAppToken{}, sql.ErrNoRows
4064+
}
4065+
40534066
func (q *FakeQuerier) GetOAuth2ProviderAppTokenByPrefix(_ context.Context, hashPrefix []byte) (database.OAuth2ProviderAppToken, error) {
40544067
q.mutex.Lock()
40554068
defer q.mutex.Unlock()
@@ -8938,13 +8951,16 @@ func (q *FakeQuerier) InsertOAuth2ProviderAppCode(_ context.Context, arg databas
89388951
for _, app := range q.oauth2ProviderApps {
89398952
if app.ID == arg.AppID {
89408953
code := database.OAuth2ProviderAppCode{
8941-
ID: arg.ID,
8942-
CreatedAt: arg.CreatedAt,
8943-
ExpiresAt: arg.ExpiresAt,
8944-
SecretPrefix: arg.SecretPrefix,
8945-
HashedSecret: arg.HashedSecret,
8946-
UserID: arg.UserID,
8947-
AppID: arg.AppID,
8954+
ID: arg.ID,
8955+
CreatedAt: arg.CreatedAt,
8956+
ExpiresAt: arg.ExpiresAt,
8957+
SecretPrefix: arg.SecretPrefix,
8958+
HashedSecret: arg.HashedSecret,
8959+
UserID: arg.UserID,
8960+
AppID: arg.AppID,
8961+
ResourceUri: arg.ResourceUri,
8962+
CodeChallenge: arg.CodeChallenge,
8963+
CodeChallengeMethod: arg.CodeChallengeMethod,
89488964
}
89498965
q.oauth2ProviderAppCodes = append(q.oauth2ProviderAppCodes, code)
89508966
return code, nil

‎coderd/database/dbmetrics/querymetrics.go

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 20 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/oauth2.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,9 @@ INSERT INTO oauth2_provider_app_tokens (
136136
-- name: GetOAuth2ProviderAppTokenByPrefix :one
137137
SELECT * FROM oauth2_provider_app_tokens WHERE hash_prefix = $1;
138138

139+
-- name: GetOAuth2ProviderAppTokenByAPIKeyID :one
140+
SELECT * FROM oauth2_provider_app_tokens WHERE api_key_id = $1;
141+
139142
-- name: GetOAuth2ProviderAppsByUserID :many
140143
SELECT
141144
COUNT(DISTINCT oauth2_provider_app_tokens.id) as token_count,

‎coderd/httpmw/apikey.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,16 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
240240
})
241241
}
242242

243+
// Validate OAuth2 provider app token audience (RFC 8707) if applicable
244+
if key.LoginType == database.LoginTypeOAuth2ProviderApp {
245+
if err := validateOAuth2ProviderAppTokenAudience(ctx, cfg.DB, *key, r); err != nil {
246+
return optionalWrite(http.StatusForbidden, codersdk.Response{
247+
Message: "Token audience mismatch",
248+
Detail: err.Error(),
249+
})
250+
}
251+
}
252+
243253
// We only check OIDC stuff if we have a valid APIKey. An expired key means we don't trust the requestor
244254
// really is the user whose key they have, and so we shouldn't be doing anything on their behalf including possibly
245255
// refreshing the OIDC token.
@@ -446,6 +456,47 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
446456
return key, &actor, true
447457
}
448458

459+
// validateOAuth2ProviderAppTokenAudience validates that an OAuth2 provider app token
460+
// is being used with the correct audience/resource server (RFC 8707).
461+
func validateOAuth2ProviderAppTokenAudience(ctx context.Context, db database.Store, key database.APIKey, r *http.Request) error {
462+
// Get the OAuth2 provider app token to check its audience
463+
//nolint:gocritic // System needs to access token for audience validation
464+
token, err := db.GetOAuth2ProviderAppTokenByAPIKeyID(dbauthz.AsSystemRestricted(ctx), key.ID)
465+
if err != nil {
466+
return xerrors.Errorf("failed to get OAuth2 token: %w", err)
467+
}
468+
469+
// If no audience is set, allow the request (for backward compatibility)
470+
if !token.Audience.Valid || token.Audience.String == "" {
471+
return nil
472+
}
473+
474+
// Extract the expected audience from the request
475+
expectedAudience := extractExpectedAudience(r)
476+
477+
// Validate that the token's audience matches the expected audience
478+
if token.Audience.String != expectedAudience {
479+
return xerrors.Errorf("token audience %q does not match expected audience %q",
480+
token.Audience.String, expectedAudience)
481+
}
482+
483+
return nil
484+
}
485+
486+
// extractExpectedAudience determines the expected audience for the current request.
487+
// This should match the resource parameter used during authorization.
488+
func extractExpectedAudience(r *http.Request) string {
489+
// For MCP compliance, the audience should be the canonical URI of the resource server
490+
// This typically matches the access URL of the Coder deployment
491+
scheme := "https"
492+
if r.TLS == nil {
493+
scheme = "http"
494+
}
495+
496+
// Use the Host header to construct the canonical audience URI
497+
return fmt.Sprintf("%s://%s", scheme, r.Host)
498+
}
499+
449500
// UserRBACSubject fetches a user's rbac.Subject from the database. It pulls all roles from both
450501
// site and organization scopes. It also pulls the groups, and the user's status.
451502
func UserRBACSubject(ctx context.Context, db database.Store, userID uuid.UUID, scope rbac.ExpandableScope) (rbac.Subject, database.UserStatus, error) {

‎coderd/identityprovider/authorize.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,15 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar
4444
codeChallenge: p.String(vals, "", "code_challenge"),
4545
codeChallengeMethod: p.String(vals, "", "code_challenge_method"),
4646
}
47+
// Validate resource indicator syntax (RFC 8707): must be absolute URI without fragment
48+
if params.resource != "" {
49+
if u, err := url.Parse(params.resource); err != nil || u.Scheme == "" || u.Fragment != "" {
50+
p.Errors = append(p.Errors, codersdk.ValidationError{
51+
Field: "resource",
52+
Detail: "must be an absolute URI without fragment",
53+
})
54+
}
55+
}
4756

4857
p.ErrorExcessParams(vals)
4958
if len(p.Errors) > 0 {

‎coderd/identityprovider/tokens.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ var (
3333
errBadToken = xerrors.New("Invalid token")
3434
// errInvalidPKCE means the PKCE verification failed.
3535
errInvalidPKCE = xerrors.New("invalid code_verifier")
36+
// errInvalidResource means the resource parameter validation failed.
37+
errInvalidResource = xerrors.New("invalid resource parameter")
3638
)
3739

3840
// OAuth2Error represents an OAuth2-compliant error response.
@@ -87,6 +89,15 @@ func extractTokenParams(r *http.Request, callbackURL *url.URL) (tokenParams, []c
8789
codeVerifier: p.String(vals, "", "code_verifier"),
8890
resource: p.String(vals, "", "resource"),
8991
}
92+
// Validate resource parameter syntax (RFC 8707): must be absolute URI without fragment
93+
if params.resource != "" {
94+
if u, err := url.Parse(params.resource); err != nil || u.Scheme == "" || u.Fragment != "" {
95+
p.Errors = append(p.Errors, codersdk.ValidationError{
96+
Field: "resource",
97+
Detail: "must be an absolute URI without fragment",
98+
})
99+
}
100+
}
90101

91102
p.ErrorExcessParams(vals)
92103
if len(p.Errors) > 0 {
@@ -153,6 +164,10 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerF
153164
writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_grant", "The PKCE code verifier is invalid")
154165
return
155166
}
167+
if errors.Is(err, errInvalidResource) {
168+
writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_target", "The resource parameter is invalid")
169+
return
170+
}
156171
if errors.Is(err, errBadToken) {
157172
writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_grant", "The refresh token is invalid or expired")
158173
return
@@ -229,6 +244,20 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
229244
}
230245
}
231246

247+
// Verify resource parameter consistency (RFC 8707)
248+
if dbCode.ResourceUri.Valid && dbCode.ResourceUri.String != "" {
249+
// Resource was specified during authorization - it must match in token request
250+
if params.resource == "" {
251+
return oauth2.Token{}, errInvalidResource
252+
}
253+
if params.resource != dbCode.ResourceUri.String {
254+
return oauth2.Token{}, errInvalidResource
255+
}
256+
} else if params.resource != "" {
257+
// Resource was not specified during authorization but is now provided
258+
return oauth2.Token{}, errInvalidResource
259+
}
260+
232261
// Generate a refresh token.
233262
refreshToken, err := GenerateSecret()
234263
if err != nil {
@@ -335,6 +364,14 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
335364
return oauth2.Token{}, errBadToken
336365
}
337366

367+
// Verify resource parameter consistency for refresh tokens (RFC 8707)
368+
if params.resource != "" {
369+
// If resource is provided in refresh request, it must match the original token's audience
370+
if !dbToken.Audience.Valid || dbToken.Audience.String != params.resource {
371+
return oauth2.Token{}, errInvalidResource
372+
}
373+
}
374+
338375
// Grab the user roles so we can perform the refresh as the user.
339376
//nolint:gocritic // There is no user yet so we must use the system.
340377
prevKey, err := db.GetAPIKeyByID(dbauthz.AsSystemRestricted(ctx), dbToken.APIKeyID)

0 commit comments

Comments
 (0)