- Notifications
You must be signed in to change notification settings - Fork926
feat: oauth2 - add RFC 8707 resource indicators and audience validation#18575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Open
ThomasK33 wants to merge3 commits intothomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_supportChoose a base branch
base:thomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
+3,422 −281
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
MemberAuthor
ThomasK33 commentedJun 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stackon Graphite.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
018694a to3daa2abComparefb90065 tod14c08eComparef4fbe1d toc8d2599Compare3daa2ab tob50e322Comparec8d2599 to4130e42Compare| // The expected behavior depends on whether the middleware detects Host differences | ||
| if _, err := client2.User(ctx, codersdk.Me); err != nil { | ||
| // This is expected if audience validation is working properly | ||
| t.Logf("Cross-resource token properly rejected: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you mean to perform an assertion here?
| func validateOAuth2ProviderAppTokenAudience(ctx context.Context, db database.Store, key database.APIKey, r *http.Request) error { | ||
| // Get the OAuth2 provider app token to check its audience | ||
| //nolint:gocritic // System needs to access token for audience validation | ||
| token, err := db.GetOAuth2ProviderAppTokenByAPIKeyID(dbauthz.AsSystemRestricted(ctx), key.ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: This is a legitimate use ofdbauthz.SystemRestricted.
Comment on lines +44 to +45
| // New OAuth2 resource-indicator methods (RFC 8707); tests to be added | ||
| "GetOAuth2ProviderAppTokenByAPIKeyID": "Not relevant", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To be added in a separate PR? Or in this PR?
| if err := validateOAuth2ProviderAppTokenAudience(ctx, cfg.DB, *key, r); err != nil { | ||
| return optionalWrite(http.StatusForbidden, codersdk.Response{ | ||
| Message: "Token audience mismatch", | ||
| Detail: err.Error(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this potentially leak information? Should we maybe log this instead but not return it?
4130e42 toe525b11Compareb50e322 to3dd6c7eComparee525b11 toaec4923Compare3dd6c7e to224784aCompareaec4923 to819ce2eCompare224784a toc2d85d9Comparec2d85d9 to69eb5c8Compare819ce2e to002ffdfCompareChange-Id: Ie024d5070dbe5901952fc52463c6602363ef8886Signed-off-by: Thomas Kosiewski <tk@coder.com>
…port- Add /.well-known/oauth-authorization-server metadata endpoint (RFC 8414)- Implement PKCE support with S256 method for enhanced security- Add resource parameter support (RFC 8707) for token binding- Add OAuth2-compliant error responses with proper error codes- Fix authorization UI to use POST-based consent instead of GET redirects- Add comprehensive OAuth2 test scripts and interactive test server- Update CLAUDE.md with OAuth2 development guidelinesDatabase changes:- Add migration 000341: code_challenge, resource_uri, audience fields- Update audit table for new OAuth2 fieldsOAuth2 provider remains development-only (requires --dev flag).Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954aSigned-off-by: Thomas Kosiewski <tk@coder.com>
Implements RFC 8707 Resource Indicators for OAuth2 provider to enable properaudience 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 protectionThis enables OAuth2 clients to specify target resource servers and preventstoken abuse across different Coder deployments through proper audience binding.Change-Id: I3924cb2139e837e3ac0b0bd40a5aeb59637ebc1bSigned-off-by: Thomas Kosiewski <tk@coder.com>
69eb5c8 to80c695bCompare002ffdf to0b43477Compare80c695b to03c4724CompareSign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
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:
/oauth2/authorize) and token (/oauth2/token) endpoints, allowing clients to specify the intended resource server.coderd/httpmw/apikey.go) to verify that the audience of the access token matches the resource server being accessed.resource_uricolumn to theoauth2_provider_app_codestable to store the resource requested during authorization.audiencecolumn to theoauth2_provider_app_tokenstable to bind the issued token to a specific audience.coderd/oauth2_test.goto cover various RFC 8707 scenarios, including valid flows, mismatched resources, and refresh token validation.How it Works:
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 Kosiewskitk@coder.com