feat: oauth2 - add RFC 8707 resource indicators and audience validation #18575
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
|
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 stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
018694a to
3daa2ab
Compare
fb90065 to
d14c08e
Compare
f4fbe1d to
c8d2599
Compare
3daa2ab to
b50e322
Compare
c8d2599 to
4130e42
Compare
johnstcn
reviewed
Jun 26, 2025
| // 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.
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.
review: This is a legitimate use of dbauthz.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.
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.
Does this potentially leak information? Should we maybe log this instead but not return it?
4130e42 to
e525b11
Compare
b50e322 to
3dd6c7e
Compare
e525b11 to
aec4923
Compare
3dd6c7e to
224784a
Compare
aec4923 to
819ce2e
Compare
224784a to
c2d85d9
Compare
c2d85d9 to
69eb5c8
Compare
819ce2e to
002ffdf
Compare
69eb5c8 to
80c695b
Compare
002ffdf to
0b43477
Compare
03c4724 to
870e5eb
Compare
058cbe7 to
495cecc
Compare
495cecc to
f46d478
Compare
870e5eb to
dd622d0
Compare
This was referenced Jun 26, 2025
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>
f46d478 to
abbe929
Compare
dd622d0 to
3092108
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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 Kosiewski tk@coder.com