Skip to content

Commit 55811a8

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 <[email protected]>
1 parent d22ac1c commit 55811a8

22 files changed

+1054
-56
lines changed

‎CLAUDE.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@ Read [cursor rules](.cursorrules).
8989
- Format: `{number}_{description}.{up|down}.sql`
9090
- Number must be unique and sequential
9191
- Always include both up and down migrations
92+
- **Use helper scripts**:
93+
- `./coderd/database/migrations/create_migration.sh "migration name"` - Creates new migration files
94+
- `./coderd/database/migrations/fix_migration_numbers.sh` - Renumbers migrations to avoid conflicts
95+
- `./coderd/database/migrations/create_fixture.sh "fixture name"` - Creates test fixtures for migrations
9296

9397
2. **Update database queries**:
9498
- MUST DO! Any changes to database - adding queries, modifying queries should be done in the `coderd/database/queries/*.sql` files
@@ -125,6 +129,29 @@ Read [cursor rules](.cursorrules).
125129
4. Run `make gen` again
126130
5. Run `make lint` to catch any remaining issues
127131

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

130157
### Core Components
@@ -209,6 +236,12 @@ When working on OAuth2 provider features:
209236
- Avoid dependency on referer headers for security decisions
210237
- Support proper state parameter validation
211238

239+
6. **RFC 8707 Resource Indicators**:
240+
- Store resource parameters in database for server-side validation (opaque tokens)
241+
- Validate resource consistency between authorization and token requests
242+
- Support audience validation in refresh token flows
243+
- Resource parameter is optional but must be consistent when provided
244+
212245
### OAuth2 Error Handling Pattern
213246

214247
```go
@@ -265,3 +298,6 @@ Always run the full test suite after OAuth2 changes:
265298
4. **Missing newlines** - Ensure files end with newline character
266299
5. **Tests passing locally but failing in CI** - Check if `dbmem` implementation needs updating
267300
6. **OAuth2 endpoints returning wrong error format** - Ensure OAuth2 endpoints return RFC 6749 compliant errors
301+
7. **OAuth2 tests failing but scripts working** - Check in-memory database implementations in `dbmem.go`
302+
8. **Resource indicator validation failing** - Ensure database stores and retrieves resource parameters correctly
303+
9. **PKCE tests failing** - Verify both authorization code storage and token exchange handle PKCE fields

‎coderd/coderd.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -781,6 +781,7 @@ func New(options *Options) *API {
781781
Optional: false,
782782
SessionTokenFunc: nil, // Default behavior
783783
PostAuthAdditionalHeadersFunc: options.PostAuthAdditionalHeadersFunc,
784+
Logger: options.Logger,
784785
})
785786
// Same as above but it redirects to the login page.
786787
apiKeyMiddlewareRedirect := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
@@ -791,6 +792,7 @@ func New(options *Options) *API {
791792
Optional: false,
792793
SessionTokenFunc: nil, // Default behavior
793794
PostAuthAdditionalHeadersFunc: options.PostAuthAdditionalHeadersFunc,
795+
Logger: options.Logger,
794796
})
795797
// Same as the first but it's optional.
796798
apiKeyMiddlewareOptional := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
@@ -801,6 +803,7 @@ func New(options *Options) *API {
801803
Optional: true,
802804
SessionTokenFunc: nil, // Default behavior
803805
PostAuthAdditionalHeadersFunc: options.PostAuthAdditionalHeadersFunc,
806+
Logger: options.Logger,
804807
})
805808

806809
workspaceAgentInfo := httpmw.ExtractWorkspaceAgentAndLatestBuild(httpmw.ExtractWorkspaceAgentAndLatestBuildConfig{

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2181,19 +2181,29 @@ func (q *querier) GetOAuth2ProviderAppSecretsByAppID(ctx context.Context, appID
21812181
return q.db.GetOAuth2ProviderAppSecretsByAppID(ctx, appID)
21822182
}
21832183

2184-
func (q *querier) GetOAuth2ProviderAppTokenByPrefix(ctx context.Context, hashPrefix []byte) (database.OAuth2ProviderAppToken, error) {
2185-
token, err := q.db.GetOAuth2ProviderAppTokenByPrefix(ctx, hashPrefix)
2184+
func (q *querier) GetOAuth2ProviderAppTokenByAPIKeyID(ctx context.Context, apiKeyID string) (database.OAuth2ProviderAppToken, error) {
2185+
token, err := q.db.GetOAuth2ProviderAppTokenByAPIKeyID(ctx, apiKeyID)
21862186
if err != nil {
21872187
return database.OAuth2ProviderAppToken{}, err
21882188
}
2189-
// The user ID is on the API key so that has to be fetched.
2190-
key, err := q.db.GetAPIKeyByID(ctx, token.APIKeyID)
2189+
2190+
if err := q.authorizeContext(ctx, policy.ActionRead, token.RBACObject()); err != nil {
2191+
return database.OAuth2ProviderAppToken{}, err
2192+
}
2193+
2194+
return token, nil
2195+
}
2196+
2197+
func (q *querier) GetOAuth2ProviderAppTokenByPrefix(ctx context.Context, hashPrefix []byte) (database.OAuth2ProviderAppToken, error) {
2198+
token, err := q.db.GetOAuth2ProviderAppTokenByPrefix(ctx, hashPrefix)
21912199
if err != nil {
21922200
return database.OAuth2ProviderAppToken{}, err
21932201
}
2194-
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOauth2AppCodeToken.WithOwner(key.UserID.String())); err != nil {
2202+
2203+
if err := q.authorizeContext(ctx, policy.ActionRead, token.RBACObject()); err != nil {
21952204
return database.OAuth2ProviderAppToken{}, err
21962205
}
2206+
21972207
return token, nil
21982208
}
21992209

@@ -3646,11 +3656,7 @@ func (q *querier) InsertOAuth2ProviderAppSecret(ctx context.Context, arg databas
36463656
}
36473657

36483658
func (q *querier) InsertOAuth2ProviderAppToken(ctx context.Context, arg database.InsertOAuth2ProviderAppTokenParams) (database.OAuth2ProviderAppToken, error) {
3649-
key, err := q.db.GetAPIKeyByID(ctx, arg.APIKeyID)
3650-
if err != nil {
3651-
return database.OAuth2ProviderAppToken{}, err
3652-
}
3653-
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceOauth2AppCodeToken.WithOwner(key.UserID.String())); err != nil {
3659+
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceOauth2AppCodeToken.WithOwner(arg.UserID.String())); err != nil {
36543660
return database.OAuth2ProviderAppToken{}, err
36553661
}
36563662
return q.db.InsertOAuth2ProviderAppToken(ctx, arg)

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5195,12 +5195,11 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
51955195
_ = dbgen.OAuth2ProviderAppToken(s.T(), db, database.OAuth2ProviderAppToken{
51965196
AppSecretID: secret.ID,
51975197
APIKeyID: key.ID,
5198+
UserID: user.ID,
51985199
HashPrefix: []byte(fmt.Sprintf("%d", i)),
51995200
})
52005201
}
52015202
expectedApp := app
5202-
expectedApp.CreatedAt = createdAt
5203-
expectedApp.UpdatedAt = createdAt
52045203
check.Args(user.ID).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionRead).Returns([]database.GetOAuth2ProviderAppsByUserIDRow{
52055204
{
52065205
OAuth2ProviderApp: expectedApp,
@@ -5363,6 +5362,7 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
53635362
check.Args(database.InsertOAuth2ProviderAppTokenParams{
53645363
AppSecretID: secret.ID,
53655364
APIKeyID: key.ID,
5365+
UserID: user.ID,
53665366
}).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionCreate)
53675367
}))
53685368
s.Run("GetOAuth2ProviderAppTokenByPrefix", s.Subtest(func(db database.Store, check *expects) {
@@ -5377,8 +5377,25 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
53775377
token := dbgen.OAuth2ProviderAppToken(s.T(), db, database.OAuth2ProviderAppToken{
53785378
AppSecretID: secret.ID,
53795379
APIKeyID: key.ID,
5380+
UserID: user.ID,
53805381
})
5381-
check.Args(token.HashPrefix).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionRead)
5382+
check.Args(token.HashPrefix).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()).WithID(token.ID), policy.ActionRead).Returns(token)
5383+
}))
5384+
s.Run("GetOAuth2ProviderAppTokenByAPIKeyID", s.Subtest(func(db database.Store, check *expects) {
5385+
user := dbgen.User(s.T(), db, database.User{})
5386+
key, _ := dbgen.APIKey(s.T(), db, database.APIKey{
5387+
UserID: user.ID,
5388+
})
5389+
app := dbgen.OAuth2ProviderApp(s.T(), db, database.OAuth2ProviderApp{})
5390+
secret := dbgen.OAuth2ProviderAppSecret(s.T(), db, database.OAuth2ProviderAppSecret{
5391+
AppID: app.ID,
5392+
})
5393+
token := dbgen.OAuth2ProviderAppToken(s.T(), db, database.OAuth2ProviderAppToken{
5394+
AppSecretID: secret.ID,
5395+
APIKeyID: key.ID,
5396+
UserID: user.ID,
5397+
})
5398+
check.Args(token.APIKeyID).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()).WithID(token.ID), policy.ActionRead).Returns(token)
53825399
}))
53835400
s.Run("DeleteOAuth2ProviderAppTokensByAppAndUserID", s.Subtest(func(db database.Store, check *expects) {
53845401
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
@@ -5394,6 +5411,7 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
53945411
_ = dbgen.OAuth2ProviderAppToken(s.T(), db, database.OAuth2ProviderAppToken{
53955412
AppSecretID: secret.ID,
53965413
APIKeyID: key.ID,
5414+
UserID: user.ID,
53975415
HashPrefix: []byte(fmt.Sprintf("%d", i)),
53985416
})
53995417
}

‎coderd/database/dbgen/dbgen.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1190,6 +1190,7 @@ func OAuth2ProviderAppToken(t testing.TB, db database.Store, seed database.OAuth
11901190
RefreshHash: takeFirstSlice(seed.RefreshHash, []byte("hashed-secret")),
11911191
AppSecretID: takeFirst(seed.AppSecretID, uuid.New()),
11921192
APIKeyID: takeFirst(seed.APIKeyID, uuid.New().String()),
1193+
UserID: takeFirst(seed.UserID, uuid.New()),
11931194
Audience: seed.Audience,
11941195
})
11951196
require.NoError(t, err, "insert oauth2 app token")

‎coderd/database/dbmem/dbmem.go

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4054,6 +4054,19 @@ func (q *FakeQuerier) GetOAuth2ProviderAppSecretsByAppID(_ context.Context, appI
40544054
return []database.OAuth2ProviderAppSecret{}, sql.ErrNoRows
40554055
}
40564056

4057+
func (q *FakeQuerier) GetOAuth2ProviderAppTokenByAPIKeyID(_ context.Context, apiKeyID string) (database.OAuth2ProviderAppToken, error) {
4058+
q.mutex.Lock()
4059+
defer q.mutex.Unlock()
4060+
4061+
for _, token := range q.oauth2ProviderAppTokens {
4062+
if token.APIKeyID == apiKeyID {
4063+
return token, nil
4064+
}
4065+
}
4066+
4067+
return database.OAuth2ProviderAppToken{}, sql.ErrNoRows
4068+
}
4069+
40574070
func (q *FakeQuerier) GetOAuth2ProviderAppTokenByPrefix(_ context.Context, hashPrefix []byte) (database.OAuth2ProviderAppToken, error) {
40584071
q.mutex.Lock()
40594072
defer q.mutex.Unlock()
@@ -4099,13 +4112,8 @@ func (q *FakeQuerier) GetOAuth2ProviderAppsByUserID(_ context.Context, userID uu
40994112
}
41004113
if len(tokens) > 0 {
41014114
rows = append(rows, database.GetOAuth2ProviderAppsByUserIDRow{
4102-
OAuth2ProviderApp: database.OAuth2ProviderApp{
4103-
CallbackURL: app.CallbackURL,
4104-
ID: app.ID,
4105-
Icon: app.Icon,
4106-
Name: app.Name,
4107-
},
4108-
TokenCount: int64(len(tokens)),
4115+
OAuth2ProviderApp: app,
4116+
TokenCount: int64(len(tokens)),
41094117
})
41104118
}
41114119
}
@@ -8918,12 +8926,15 @@ func (q *FakeQuerier) InsertOAuth2ProviderApp(_ context.Context, arg database.In
89188926

89198927
//nolint:gosimple // Go wants database.OAuth2ProviderApp(arg), but we cannot be sure the structs will remain identical.
89208928
app := database.OAuth2ProviderApp{
8921-
ID: arg.ID,
8922-
CreatedAt: arg.CreatedAt,
8923-
UpdatedAt: arg.UpdatedAt,
8924-
Name: arg.Name,
8925-
Icon: arg.Icon,
8926-
CallbackURL: arg.CallbackURL,
8929+
ID: arg.ID,
8930+
CreatedAt: arg.CreatedAt,
8931+
UpdatedAt: arg.UpdatedAt,
8932+
Name: arg.Name,
8933+
Icon: arg.Icon,
8934+
CallbackURL: arg.CallbackURL,
8935+
RedirectUris: arg.RedirectUris,
8936+
ClientType: arg.ClientType,
8937+
DynamicallyRegistered: arg.DynamicallyRegistered,
89278938
}
89288939
q.oauth2ProviderApps = append(q.oauth2ProviderApps, app)
89298940

@@ -9008,6 +9019,8 @@ func (q *FakeQuerier) InsertOAuth2ProviderAppToken(_ context.Context, arg databa
90089019
RefreshHash: arg.RefreshHash,
90099020
APIKeyID: arg.APIKeyID,
90109021
AppSecretID: arg.AppSecretID,
9022+
UserID: arg.UserID,
9023+
Audience: arg.Audience,
90119024
}
90129025
q.oauth2ProviderAppTokens = append(q.oauth2ProviderAppTokens, token)
90139026
return token, nil
@@ -10790,12 +10803,15 @@ func (q *FakeQuerier) UpdateOAuth2ProviderAppByID(_ context.Context, arg databas
1079010803
for index, app := range q.oauth2ProviderApps {
1079110804
if app.ID == arg.ID {
1079210805
newApp := database.OAuth2ProviderApp{
10793-
ID: arg.ID,
10794-
CreatedAt: app.CreatedAt,
10795-
UpdatedAt: arg.UpdatedAt,
10796-
Name: arg.Name,
10797-
Icon: arg.Icon,
10798-
CallbackURL: arg.CallbackURL,
10806+
ID: arg.ID,
10807+
CreatedAt: app.CreatedAt,
10808+
UpdatedAt: arg.UpdatedAt,
10809+
Name: arg.Name,
10810+
Icon: arg.Icon,
10811+
CallbackURL: arg.CallbackURL,
10812+
RedirectUris: arg.RedirectUris,
10813+
ClientType: arg.ClientType,
10814+
DynamicallyRegistered: arg.DynamicallyRegistered,
1079910815
}
1080010816
q.oauth2ProviderApps[index] = newApp
1080110817
return newApp, 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/dump.sql

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

‎coderd/database/foreign_key_constraint.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/migrations/000345_oauth2_provider_app_tokens_denormalize_user_id.down.sql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
-- Remove the denormalized user_id column from oauth2_provider_app_tokens
2+
ALTER TABLE oauth2_provider_app_tokens
3+
DROP CONSTRAINT IF EXISTS fk_oauth2_provider_app_tokens_user_id;
4+
5+
ALTER TABLE oauth2_provider_app_tokens
6+
DROP COLUMN IF EXISTS user_id;

‎coderd/database/migrations/000345_oauth2_provider_app_tokens_denormalize_user_id.up.sql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
-- Add user_id column to oauth2_provider_app_tokens for performance optimization
2+
-- This eliminates the need to join with api_keys table for authorization checks
3+
ALTER TABLE oauth2_provider_app_tokens
4+
ADD COLUMN user_id uuid;
5+
6+
-- Backfill existing records with user_id from the associated api_key
7+
UPDATE oauth2_provider_app_tokens
8+
SET user_id = api_keys.user_id
9+
FROM api_keys
10+
WHERE oauth2_provider_app_tokens.api_key_id = api_keys.id;
11+
12+
-- Make user_id NOT NULL after backfilling
13+
ALTER TABLE oauth2_provider_app_tokens
14+
ALTER COLUMN user_id SET NOT NULL;
15+
16+
-- Add foreign key constraint to maintain referential integrity
17+
ALTER TABLE oauth2_provider_app_tokens
18+
ADD CONSTRAINT fk_oauth2_provider_app_tokens_user_id
19+
FOREIGN KEY (user_id) REFERENCES users (id) ON DELETE CASCADE;
20+
21+
COMMENT ON COLUMN oauth2_provider_app_tokens.user_id IS 'Denormalized user ID for performance optimization in authorization checks';

0 commit comments

Comments
 (0)