Skip to content

Commit 6f2834f

Browse files
ThomasK33ThomasK33
authored
feat: oauth2 - add authorization server metadata endpoint and PKCE support (#18548)
## Summary This PR implements critical MCP OAuth2 compliance features for Coder's authorization server, adding PKCE support, resource parameter handling, and OAuth2 server metadata discovery. This brings Coder's OAuth2 implementation significantly closer to production readiness for MCP (Model Context Protocol) integrations. ## What's Added ### OAuth2 Authorization Server Metadata (RFC 8414) - Add `/.well-known/oauth-authorization-server` endpoint for automatic client discovery - Returns standardized metadata including supported grant types, response types, and PKCE methods - Essential for MCP client compatibility and OAuth2 standards compliance ### PKCE Support (RFC 7636) - Implement Proof Key for Code Exchange with S256 challenge method - Add `code_challenge` and `code_challenge_method` parameters to authorization flow - Add `code_verifier` validation in token exchange - Provides enhanced security for public clients (mobile apps, CLIs) ### Resource Parameter Support (RFC 8707) - Add `resource` parameter to authorization and token endpoints - Store resource URI and bind tokens to specific audiences - Critical for MCP's resource-bound token model ### Enhanced OAuth2 Error Handling - Add OAuth2-compliant error responses with proper error codes - Use standard error format: `{"error": "code", "error_description": "details"}` - Improve error consistency across OAuth2 endpoints ### Authorization UI Improvements - Fix authorization flow to use POST-based consent instead of GET redirects - Remove dependency on referer headers for security decisions - Improve CSRF protection with proper state parameter validation ## Why This Matters **For MCP Integration:** MCP requires OAuth2 authorization servers to support PKCE, resource parameters, and metadata discovery. Without these features, MCP clients cannot securely authenticate with Coder. **For Security:** PKCE prevents authorization code interception attacks, especially critical for public clients. Resource binding ensures tokens are only valid for intended services. **For Standards Compliance:** These are widely adopted OAuth2 extensions that improve interoperability with modern OAuth2 clients. ## Database Changes - **Migration 000343:** Adds `code_challenge`, `code_challenge_method`, `resource_uri` to `oauth2_provider_app_codes` - **Migration 000343:** Adds `audience` field to `oauth2_provider_app_tokens` for resource binding - **Audit Updates:** New OAuth2 fields properly tracked in audit system - **Backward Compatibility:** All changes maintain compatibility with existing OAuth2 flows ## Test Coverage - Comprehensive PKCE test suite in `coderd/identityprovider/pkce_test.go` - OAuth2 metadata endpoint tests in `coderd/oauth2_metadata_test.go` - Integration tests covering PKCE + resource parameter combinations - Negative tests for invalid PKCE verifiers and malformed requests ## Testing Instructions ```bash # Run the comprehensive OAuth2 test suite ./scripts/oauth2/test-mcp-oauth2.sh Manual Testing with Interactive Server # Start Coder in development mode ./scripts/develop.sh # In another terminal, set up test app and run interactive flow eval $(./scripts/oauth2/setup-test-app.sh) ./scripts/oauth2/test-manual-flow.sh # Opens browser with OAuth2 flow, handles callback automatically # Clean up when done ./scripts/oauth2/cleanup-test-app.sh Individual Component Testing # Test metadata endpoint curl -s http://localhost:3000/.well-known/oauth-authorization-server | jq . # Test PKCE generation ./scripts/oauth2/generate-pkce.sh # Run specific test suites go test -v ./coderd/identityprovider -run TestVerifyPKCE go test -v ./coderd -run TestOAuth2AuthorizationServerMetadata ``` ### Breaking Changes None. All changes maintain backward compatibility with existing OAuth2 flows. --- Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954a Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent dbfbef6 commit 6f2834f

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+2846
-304
lines changed

‎CLAUDE.md

Lines changed: 165 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,22 @@
22

33
Read [cursor rules](.cursorrules).
44

5+
## Quick Start Checklist for New Features
6+
7+
### Before Starting
8+
9+
- [ ] Run `git pull` to ensure you're on latest code
10+
- [ ] Check if feature touches database - you'll need migrations
11+
- [ ] Check if feature touches audit logs - update `enterprise/audit/table.go`
12+
13+
## Development Server
14+
15+
### Starting Development Mode
16+
17+
- Use `./scripts/develop.sh` to start Coder in development mode
18+
- This automatically builds and runs with `--dev` flag and proper access URL
19+
- Do NOT manually run `make build && ./coder server --dev` - use the script instead
20+
521
## Build/Test/Lint Commands
622

723
### Main Commands
@@ -34,6 +50,7 @@ Read [cursor rules](.cursorrules).
3450
- Use `gofumpt` for formatting
3551
- Create packages when used during implementation
3652
- Validate abstractions against implementations
53+
- **Test packages**: Use `package_test` naming (e.g., `identityprovider_test`) for black-box testing
3754

3855
### Error Handling
3956

@@ -63,11 +80,50 @@ Read [cursor rules](.cursorrules).
6380
- Keep message titles concise (~70 characters)
6481
- Use imperative, present tense in commit titles
6582

66-
## Database queries
83+
## Database Work
84+
85+
### Migration Guidelines
86+
87+
1. **Create migration files**:
88+
- Location: `coderd/database/migrations/`
89+
- Format: `{number}_{description}.{up|down}.sql`
90+
- Number must be unique and sequential
91+
- Always include both up and down migrations
92+
93+
2. **Update database queries**:
94+
- MUST DO! Any changes to database - adding queries, modifying queries should be done in the `coderd/database/queries/*.sql` files
95+
- MUST DO! Queries are grouped in files relating to context - e.g. `prebuilds.sql`, `users.sql`, `oauth2.sql`
96+
- After making changes to any `coderd/database/queries/*.sql` files you must run `make gen` to generate respective ORM changes
97+
98+
3. **Handle nullable fields**:
99+
- Use `sql.NullString`, `sql.NullBool`, etc. for optional database fields
100+
- Set `.Valid = true` when providing values
101+
- Example:
102+
103+
```go
104+
CodeChallenge: sql.NullString{
105+
String: params.codeChallenge,
106+
Valid: params.codeChallenge != "",
107+
}
108+
```
67109

68-
- MUST DO! Any changes to database - adding queries, modifying queries should be done in the `coderd\database\queries\*.sql` files. Use `make gen` to generate necessary changes after.
69-
- MUST DO! Queries are grouped in files relating to context - e.g. `prebuilds.sql`, `users.sql`, `provisionerjobs.sql`.
70-
- After making changes to any `coderd\database\queries\*.sql` files you must run `make gen` to generate respective ORM changes.
110+
4. **Audit table updates**:
111+
- If adding fields to auditable types, update `enterprise/audit/table.go`
112+
- Add each new field with appropriate action (ActionTrack, ActionIgnore, ActionSecret)
113+
- Run `make gen` to verify no audit errors
114+
115+
5. **In-memory database (dbmem) updates**:
116+
- When adding new fields to database structs, ensure `dbmem` implementation copies all fields
117+
- Check `coderd/database/dbmem/dbmem.go` for Insert/Update methods
118+
- Missing fields in dbmem can cause tests to fail even if main implementation is correct
119+
120+
### Database Generation Process
121+
122+
1. Modify SQL files in `coderd/database/queries/`
123+
2. Run `make gen`
124+
3. If errors about audit table, update `enterprise/audit/table.go`
125+
4. Run `make gen` again
126+
5. Run `make lint` to catch any remaining issues
71127

72128
## Architecture
73129

@@ -78,6 +134,14 @@ Read [cursor rules](.cursorrules).
78134
- **Agents**: Services in remote workspaces providing features like SSH and port forwarding
79135
- **Workspaces**: Cloud resources defined by Terraform
80136

137+
### Adding New API Endpoints
138+
139+
1. **Define types** in `codersdk/` package
140+
2. **Add handler** in appropriate `coderd/` file
141+
3. **Register route** in `coderd/coderd.go`
142+
4. **Add tests** in `coderd/*_test.go` files
143+
5. **Update OpenAPI** by running `make gen`
144+
81145
## Sub-modules
82146

83147
### Template System
@@ -104,3 +168,100 @@ Read [cursor rules](.cursorrules).
104168
The frontend is contained in the site folder.
105169

106170
For building Frontend refer to [this document](docs/about/contributing/frontend.md)
171+
172+
## Common Patterns
173+
174+
### OAuth2/Authentication Work
175+
176+
- Types go in `codersdk/oauth2.go` or similar
177+
- Handlers go in `coderd/oauth2.go` or `coderd/identityprovider/`
178+
- Database fields need migration + audit table updates
179+
- Always support backward compatibility
180+
181+
## OAuth2 Development
182+
183+
### OAuth2 Provider Implementation
184+
185+
When working on OAuth2 provider features:
186+
187+
1. **OAuth2 Spec Compliance**:
188+
- Follow RFC 6749 for token responses
189+
- Use `expires_in` (seconds) not `expiry` (timestamp) in token responses
190+
- Return proper OAuth2 error format: `{"error": "code", "error_description": "details"}`
191+
192+
2. **Error Response Format**:
193+
- Create OAuth2-compliant error responses for token endpoint
194+
- Use standard error codes: `invalid_client`, `invalid_grant`, `invalid_request`
195+
- Avoid generic error responses for OAuth2 endpoints
196+
197+
3. **Testing OAuth2 Features**:
198+
- Use scripts in `./scripts/oauth2/` for testing
199+
- Run `./scripts/oauth2/test-mcp-oauth2.sh` for comprehensive tests
200+
- Manual testing: use `./scripts/oauth2/test-manual-flow.sh`
201+
202+
4. **PKCE Implementation**:
203+
- Support both with and without PKCE for backward compatibility
204+
- Use S256 method for code challenge
205+
- Properly validate code_verifier against stored code_challenge
206+
207+
5. **UI Authorization Flow**:
208+
- Use POST requests for consent, not GET with links
209+
- Avoid dependency on referer headers for security decisions
210+
- Support proper state parameter validation
211+
212+
### OAuth2 Error Handling Pattern
213+
214+
```go
215+
// Define specific OAuth2 errors
216+
var (
217+
errInvalidPKCE = xerrors.New("invalid code_verifier")
218+
)
219+
220+
// Use OAuth2-compliant error responses
221+
type OAuth2Error struct {
222+
Error string `json:"error"`
223+
ErrorDescription string `json:"error_description,omitempty"`
224+
}
225+
226+
// Return proper OAuth2 errors
227+
if errors.Is(err, errInvalidPKCE) {
228+
writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_grant", "The PKCE code verifier is invalid")
229+
return
230+
}
231+
```
232+
233+
### Testing Patterns
234+
235+
- Use table-driven tests for comprehensive coverage
236+
- Mock external dependencies
237+
- Test both positive and negative cases
238+
- Use `testutil.WaitLong` for timeouts in tests
239+
240+
## Testing Scripts
241+
242+
### OAuth2 Test Scripts
243+
244+
Located in `./scripts/oauth2/`:
245+
246+
- `test-mcp-oauth2.sh` - Full automated test suite
247+
- `setup-test-app.sh` - Create test OAuth2 app
248+
- `cleanup-test-app.sh` - Remove test app
249+
- `generate-pkce.sh` - Generate PKCE parameters
250+
- `test-manual-flow.sh` - Manual browser testing
251+
252+
Always run the full test suite after OAuth2 changes:
253+
254+
```bash
255+
./scripts/oauth2/test-mcp-oauth2.sh
256+
```
257+
258+
## Troubleshooting
259+
260+
### Common Issues
261+
262+
1. **"Audit table entry missing action"** - Update `enterprise/audit/table.go`
263+
2. **"package should be X_test"** - Use `package_test` naming for test files
264+
3. **SQL type errors** - Use `sql.Null*` types for nullable fields
265+
4. **Missing newlines** - Ensure files end with newline character
266+
5. **Tests passing locally but failing in CI** - Check if `dbmem` implementation needs updating
267+
6. **OAuth2 endpoints returning wrong error format** - Ensure OAuth2 endpoints return RFC 6749 compliant errors

‎coderd/apidoc/docs.go

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

0 commit comments

Comments
 (0)