Skip to content

Commit f0c9c4d

Browse files
ThomasK33ThomasK33
authored
feat: oauth2 - add RFC 8707 resource indicators and audience validation (#18575)
This pull request implements RFC 8707, Resource Indicators for OAuth 2.0 (https://datatracker.ietf.org/doc/html/rfc8707), to enhance the security of our OAuth 2.0 provider. This change enables proper audience validation and binds access tokens to their intended resource, which is crucial for preventing token misuse in multi-tenant environments or deployments with multiple resource servers. ## Key Changes: * Resource Parameter Support: Adds support for the resource parameter in both the authorization (`/oauth2/authorize`) and token (`/oauth2/token`) endpoints, allowing clients to specify the intended resource server. * Audience Validation: Implements server-side validation to ensure that the resource parameter provided during the token exchange matches the one from the authorization request. * API Middleware Enforcement: Introduces a new validation step in the API authentication middleware (`coderd/httpmw/apikey.go`) to verify that the audience of the access token matches the resource server being accessed. * Database Schema Updates: * Adds a `resource_uri` column to the `oauth2_provider_app_codes` table to store the resource requested during authorization. * Adds an `audience` column to the `oauth2_provider_app_tokens` table to bind the issued token to a specific audience. * Enhanced PKCE: Includes a minor enhancement to the PKCE implementation to protect against timing attacks. * Comprehensive Testing: Adds extensive new tests to `coderd/oauth2_test.go` to cover various RFC 8707 scenarios, including valid flows, mismatched resources, and refresh token validation. ## How it Works: 1. An OAuth2 client specifies the target resource (e.g., https://coder.example.com) using the resource parameter in the authorization request. 2. The authorization server stores this resource URI with the authorization code. 3. During the token exchange, the server validates that the client provides the same resource parameter. 4. The server issues an access token with an audience claim set to the validated resource URI. 5. When the client uses the access token to call an API endpoint, the middleware verifies that the token's audience matches the URL of the Coder deployment, rejecting any tokens intended for a different resource. This ensures that a token issued for one Coder deployment cannot be used to access another, significantly strengthening our authentication security. --- Change-Id: I3924cb2139e837e3ac0b0bd40a5aeb59637ebc1b Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent 01163ea commit f0c9c4d

22 files changed

+1007
-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

@@ -3650,11 +3660,7 @@ func (q *querier) InsertOAuth2ProviderAppSecret(ctx context.Context, arg databas
36503660
}
36513661

36523662
func (q *querier) InsertOAuth2ProviderAppToken(ctx context.Context, arg database.InsertOAuth2ProviderAppTokenParams) (database.OAuth2ProviderAppToken, error) {
3653-
key, err := q.db.GetAPIKeyByID(ctx, arg.APIKeyID)
3654-
if err != nil {
3655-
return database.OAuth2ProviderAppToken{}, err
3656-
}
3657-
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceOauth2AppCodeToken.WithOwner(key.UserID.String())); err != nil {
3663+
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceOauth2AppCodeToken.WithOwner(arg.UserID.String())); err != nil {
36583664
return database.OAuth2ProviderAppToken{}, err
36593665
}
36603666
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
@@ -5201,12 +5201,11 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
52015201
_ = dbgen.OAuth2ProviderAppToken(s.T(), db, database.OAuth2ProviderAppToken{
52025202
AppSecretID: secret.ID,
52035203
APIKeyID: key.ID,
5204+
UserID: user.ID,
52045205
HashPrefix: []byte(fmt.Sprintf("%d", i)),
52055206
})
52065207
}
52075208
expectedApp := app
5208-
expectedApp.CreatedAt = createdAt
5209-
expectedApp.UpdatedAt = createdAt
52105209
check.Args(user.ID).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionRead).Returns([]database.GetOAuth2ProviderAppsByUserIDRow{
52115210
{
52125211
OAuth2ProviderApp: expectedApp,
@@ -5369,6 +5368,7 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
53695368
check.Args(database.InsertOAuth2ProviderAppTokenParams{
53705369
AppSecretID: secret.ID,
53715370
APIKeyID: key.ID,
5371+
UserID: user.ID,
53725372
}).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionCreate)
53735373
}))
53745374
s.Run("GetOAuth2ProviderAppTokenByPrefix", s.Subtest(func(db database.Store, check *expects) {
@@ -5383,8 +5383,25 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
53835383
token := dbgen.OAuth2ProviderAppToken(s.T(), db, database.OAuth2ProviderAppToken{
53845384
AppSecretID: secret.ID,
53855385
APIKeyID: key.ID,
5386+
UserID: user.ID,
53865387
})
5387-
check.Args(token.HashPrefix).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionRead)
5388+
check.Args(token.HashPrefix).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()).WithID(token.ID), policy.ActionRead).Returns(token)
5389+
}))
5390+
s.Run("GetOAuth2ProviderAppTokenByAPIKeyID", s.Subtest(func(db database.Store, check *expects) {
5391+
user := dbgen.User(s.T(), db, database.User{})
5392+
key, _ := dbgen.APIKey(s.T(), db, database.APIKey{
5393+
UserID: user.ID,
5394+
})
5395+
app := dbgen.OAuth2ProviderApp(s.T(), db, database.OAuth2ProviderApp{})
5396+
secret := dbgen.OAuth2ProviderAppSecret(s.T(), db, database.OAuth2ProviderAppSecret{
5397+
AppID: app.ID,
5398+
})
5399+
token := dbgen.OAuth2ProviderAppToken(s.T(), db, database.OAuth2ProviderAppToken{
5400+
AppSecretID: secret.ID,
5401+
APIKeyID: key.ID,
5402+
UserID: user.ID,
5403+
})
5404+
check.Args(token.APIKeyID).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()).WithID(token.ID), policy.ActionRead).Returns(token)
53885405
}))
53895406
s.Run("DeleteOAuth2ProviderAppTokensByAppAndUserID", s.Subtest(func(db database.Store, check *expects) {
53905407
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
@@ -5400,6 +5417,7 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
54005417
_ = dbgen.OAuth2ProviderAppToken(s.T(), db, database.OAuth2ProviderAppToken{
54015418
AppSecretID: secret.ID,
54025419
APIKeyID: key.ID,
5420+
UserID: user.ID,
54035421
HashPrefix: []byte(fmt.Sprintf("%d", i)),
54045422
})
54055423
}

‎coderd/database/dbgen/dbgen.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,6 +1191,7 @@ func OAuth2ProviderAppToken(t testing.TB, db database.Store, seed database.OAuth
11911191
RefreshHash: takeFirstSlice(seed.RefreshHash, []byte("hashed-secret")),
11921192
AppSecretID: takeFirst(seed.AppSecretID, uuid.New()),
11931193
APIKeyID: takeFirst(seed.APIKeyID, uuid.New().String()),
1194+
UserID: takeFirst(seed.UserID, uuid.New()),
11941195
Audience: seed.Audience,
11951196
})
11961197
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
@@ -4055,6 +4055,19 @@ func (q *FakeQuerier) GetOAuth2ProviderAppSecretsByAppID(_ context.Context, appI
40554055
return []database.OAuth2ProviderAppSecret{}, sql.ErrNoRows
40564056
}
40574057

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

89278935
//nolint:gosimple // Go wants database.OAuth2ProviderApp(arg), but we cannot be sure the structs will remain identical.
89288936
app := database.OAuth2ProviderApp{
8929-
ID: arg.ID,
8930-
CreatedAt: arg.CreatedAt,
8931-
UpdatedAt: arg.UpdatedAt,
8932-
Name: arg.Name,
8933-
Icon: arg.Icon,
8934-
CallbackURL: arg.CallbackURL,
8937+
ID: arg.ID,
8938+
CreatedAt: arg.CreatedAt,
8939+
UpdatedAt: arg.UpdatedAt,
8940+
Name: arg.Name,
8941+
Icon: arg.Icon,
8942+
CallbackURL: arg.CallbackURL,
8943+
RedirectUris: arg.RedirectUris,
8944+
ClientType: arg.ClientType,
8945+
DynamicallyRegistered: arg.DynamicallyRegistered,
89358946
}
89368947
q.oauth2ProviderApps = append(q.oauth2ProviderApps, app)
89378948

@@ -9016,6 +9027,8 @@ func (q *FakeQuerier) InsertOAuth2ProviderAppToken(_ context.Context, arg databa
90169027
RefreshHash: arg.RefreshHash,
90179028
APIKeyID: arg.APIKeyID,
90189029
AppSecretID: arg.AppSecretID,
9030+
UserID: arg.UserID,
9031+
Audience: arg.Audience,
90199032
}
90209033
q.oauth2ProviderAppTokens = append(q.oauth2ProviderAppTokens, token)
90219034
return token, nil
@@ -10798,12 +10811,15 @@ func (q *FakeQuerier) UpdateOAuth2ProviderAppByID(_ context.Context, arg databas
1079810811
for index, app := range q.oauth2ProviderApps {
1079910812
if app.ID == arg.ID {
1080010813
newApp := database.OAuth2ProviderApp{
10801-
ID: arg.ID,
10802-
CreatedAt: app.CreatedAt,
10803-
UpdatedAt: arg.UpdatedAt,
10804-
Name: arg.Name,
10805-
Icon: arg.Icon,
10806-
CallbackURL: arg.CallbackURL,
10814+
ID: arg.ID,
10815+
CreatedAt: app.CreatedAt,
10816+
UpdatedAt: arg.UpdatedAt,
10817+
Name: arg.Name,
10818+
Icon: arg.Icon,
10819+
CallbackURL: arg.CallbackURL,
10820+
RedirectUris: arg.RedirectUris,
10821+
ClientType: arg.ClientType,
10822+
DynamicallyRegistered: arg.DynamicallyRegistered,
1080710823
}
1080810824
q.oauth2ProviderApps[index] = newApp
1080910825
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)