fix OIDC introspection authentication #31632

slingamn · 2024-07-15T06:15:53Z

See discussion on #31561 for some background.

The introspect endpoint was using the OIDC token itself for
authentication. This fixes it to use basic authentication with the
client ID and secret instead:

Applications with a valid client ID and secret should be able to
successfully introspect an invalid token, receiving a 200 response
with JSON data that indicates the token is invalid
Requests with an invalid client ID and secret should not be able
to introspect, even if the token itself is valid

Unlike #31561 (which just future-proofed the current behavior against future changes to DISABLE_QUERY_AUTH_TOKEN), this is a potential compatibility break (some introspection requests without valid client IDs that would previously succeed will now fail). Affected deployments must begin sending a valid HTTP basic authentication header with their introspection requests, with the username set to a valid client ID and the password set to the corresponding client secret.

The introspect endpoint was using the OIDC token itself for authentication. This fixes it to use basic authentication with the client ID and secret instead: * Applications with a valid client ID and secret should be able to successfully introspect an invalid token, receiving a 200 response with JSON data that indicates the token is invalid * Requests with an invalid client ID and secret should not be able to introspect, even if the token itself is valid

routers/web/auth/oauth.go

Co-authored-by: Lauris BH <[email protected]>

routers/web/auth/oauth.go

tests/integration/oauth_test.go

routers/web/auth/oauth.go

slingamn · 2024-07-18T22:25:51Z

@techknowlogick re. your question on #31632 about integration testing, I think the automated integration test added here is a pretty good description of the flow: first you issue a token from /login/oauth/access_token, then you pass it to /login/oauth/introspect and get back the JSON introspection response.

I did a successful end-to-end test of an IRCv3 integration (it needed one additional patch, to add the username field to the introspection response, that I plan to submit separately later).

delvh · 2024-07-19T20:49:42Z

Why is this PR breaking?
Please describe in the PR description why this PR is breaking, and what affected users need to do.

slingamn · 2024-07-19T23:05:42Z

I updated the description. tl;dr after this change, clients using the introspection endpoint will need to start sending a valid basic auth header.

I could also remove the basic auth check. Okta's rationale for it doesn't make sense to me:

If the introspection endpoint is left open and un-throttled, it presents a means for an attacker to poll the endpoint fishing for a valid token. To prevent this, the server must either require authentication of the clients using the endpoint, or only make the endpoint available to internal servers through other means such as a firewall. Note that the resources servers are also a potential target of a fishing attack, and should take countermeasures such as rate limiting to prevent this.

The tokens here are JWTs so they can't be brute-forced, and I don't think DoS is a real concern either. But I would like someone with more OIDC expertise to weigh in on this :-)

techknowlogick · 2024-07-19T23:42:39Z

While rate-limiting is definitely a concern, it can be considered as out-of-scope for this PR. I also am not sure we need to be concerned with this opening up a venue for malicious users to see if a token is valid or not, as that can essentially be done with any endpoint.
Please let me know if I've misunderstood your latest post.
Thanks for your focus on improving the oauth experience :)

lafriks · 2024-07-20T14:36:24Z

Usually introspection tokens are not jwt tokens so they cannot be validated and introspection endpoint is used to do that and that's why it requires separate auth so that only valid clients can do that. On the other hand even if jwt token is used introspection endpoint can be used to check if session is still valid even if jwt token is not expired itself

slingamn · 2024-07-22T06:59:48Z

Just checking if there are still any open issues blocking a merge?

* origin/main: (59 commits) fix OIDC introspection authentication (go-gitea#31632) Enable direnv (go-gitea#31672) [skip ci] Updated translations via Crowdin [skip ci] Updated translations via Crowdin fix redis dep (go-gitea#31662) add skip secondary authorization option for public oauth2 clients (go-gitea#31454) Fix a branch divergence cache bug (go-gitea#31659) [skip ci] Updated translations via Crowdin Remove unneccessary uses of `word-break: break-all` (go-gitea#31637) [skip ci] Updated translations via Crowdin Allow searching issues by ID (go-gitea#31479) allow synchronizing user status from OAuth2 login providers (go-gitea#31572) Enable `no-jquery/no-class-state` (go-gitea#31639) Added default sorting milestones by name (go-gitea#27084) Code editor theme enhancements (go-gitea#31629) Add option to change mail from user display name (go-gitea#31528) Upgrade xorm to v1.3.9 and improve some migrations Sync (go-gitea#29899) Issue Templates: add option to have dropdown printed list (go-gitea#31577) Fix update flake (go-gitea#31626) [skip ci] Updated translations via Crowdin ...

* giteaofficial/main: use nolyfill to remove some polyfills (go-gitea#31468) Properly filter issue list given no assignees filter (go-gitea#31522) Run `detectWebAuthnSupport` only on sign-in page (go-gitea#31676) fix OIDC introspection authentication (go-gitea#31632) Enable direnv (go-gitea#31672) [skip ci] Updated translations via Crowdin [skip ci] Updated translations via Crowdin

kylef · 2024-07-25T08:13:20Z

routers/web/auth/oauth.go

@@ -326,10 +325,29 @@ func getOAuthGroupsForUser(ctx go_context.Context, user *user_model.User) ([]str
 	return groups, nil
 }

+func parseBasicAuth(ctx *context.Context) (username, password string, err error) {
+	authHeader := ctx.Req.Header.Get("Authorization")
+	if authType, authData, ok := strings.Cut(authHeader, " "); ok && authType == "Basic" {


I believe auth scheme "Basic" should be compare case insensitively

"Note that both scheme and parameter names are matched case-insensitively."
https://www.rfc-editor.org/rfc/rfc7617.html#section-2

Good catch. @slingamn was that a deliberate change or we just missed it in reviewing?

Wasn't intentional :-) I just refactored the original implementation (#6293) which was case-sensitive. So there's no behavior change, but I agree that a case-insensitive comparison would be more correct.

@kylef

@kylef pointed out on #31632 that [RFC7617](https://www.rfc-editor.org/rfc/rfc7617.html#section-2) mandates case-insensitive comparison of the scheme field `Basic`. #31632 copied a case-sensitive comparison from #6293. This PR fixes both comparisons. The issue only affects OIDC, since the implementation for normal Gitea endpoints is already correct: https://github.com/go-gitea/gitea/blob/930ca92d7ce80e8b0bdaf92e495026baf2a1d419/services/auth/basic.go#L55-L58

@earl-warren

These are the three conflicted changes from #4716: * go-gitea/gitea#31632 * go-gitea/gitea#31688 * go-gitea/gitea#31706 cc @earl-warren; as per discussion on go-gitea/gitea#31632 this involves a small compatibility break (OIDC introspection requests now require a valid client ID and secret, instead of a valid OIDC token) ## Checklist The [developer guide](https://forgejo.org/docs/next/developer/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [ ] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [ ] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.  ## Draft release notes  - Breaking features - [PR](https://codeberg.org/forgejo/forgejo/pulls/4724): OIDC integrations that POST to `/login/oauth/introspect` without sending HTTP basic authentication will now fail with a 401 HTTP Unauthorized error. To fix the error, the client must begin sending HTTP basic authentication with a valid client ID and secret. This endpoint was previously authenticated via the introspection token itself, which is less secure.  Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4724 Reviewed-by: Earl Warren <[email protected]> Co-authored-by: Shivaram Lingamneni <[email protected]> Co-committed-by: Shivaram Lingamneni <[email protected]>

slingamn added 2 commits July 15, 2024 05:51

add an integration test for OIDC introspection

7efd88a

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 15, 2024

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 15, 2024

github-actions bot added the modifies/go Pull requests that update Go code label Jul 15, 2024

slingamn mentioned this pull request Jul 15, 2024

allow token parameter from the POST body #31561

Closed

lafriks requested changes Jul 15, 2024

View reviewed changes

routers/web/auth/oauth.go Outdated Show resolved Hide resolved

GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 15, 2024

review fix: use strings.Cut

b948547

slingamn requested a review from lafriks July 15, 2024 06:41

fix non-confidential access_token flow

0dee9ce

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 15, 2024

lafriks reviewed Jul 15, 2024

View reviewed changes

routers/web/auth/oauth.go Outdated Show resolved Hide resolved

techknowlogick added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Jul 15, 2024

Update routers/web/auth/oauth.go

a17da3e

Co-authored-by: Lauris BH <[email protected]>

bohde suggested changes Jul 15, 2024

View reviewed changes

routers/web/auth/oauth.go Outdated Show resolved Hide resolved

tests/integration/oauth_test.go Show resolved Hide resolved

slingamn added 4 commits July 15, 2024 16:38

use strings.Cut for BasicAuthDecode as well

6740a45

review fix: return a 500 for database errors

6f8573f

review fix: add a test for invalid client secret

f5a45c3

review fix: add a test for successfully introspecting an invalid token

e160947

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 16, 2024

slingamn requested a review from lafriks July 16, 2024 05:16

lafriks reviewed Jul 16, 2024

View reviewed changes

routers/web/auth/oauth.go Outdated Show resolved Hide resolved

slingamn requested a review from bohde July 16, 2024 14:34

bohde approved these changes Jul 17, 2024

View reviewed changes

review fix: refactor error handling control flow

21978a2

lafriks approved these changes Jul 17, 2024

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Jul 17, 2024

Merge branch 'main' into oauth_introspection_auth

9cd8ac3

lafriks approved these changes Jul 20, 2024

View reviewed changes

Merge branch 'main' into oauth_introspection_auth

e732f56

6543 enabled auto-merge (squash) July 23, 2024 12:13

6543 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 23, 2024

Merge branch 'main' into oauth_introspection_auth

205dc77

6543 merged commit 2f1cb1d into go-gitea:main Jul 23, 2024
26 checks passed

GiteaBot added this to the 1.23.0 milestone Jul 23, 2024

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 23, 2024

kylef reviewed Jul 25, 2024

View reviewed changes

slingamn mentioned this pull request Jul 26, 2024

OIDC: case-insensitive comparison for auth scheme Basic #31706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix OIDC introspection authentication #31632

fix OIDC introspection authentication #31632

slingamn commented Jul 15, 2024 •

edited

Loading

slingamn commented Jul 18, 2024

delvh commented Jul 19, 2024 •

edited

Loading

slingamn commented Jul 19, 2024

techknowlogick commented Jul 19, 2024

lafriks commented Jul 20, 2024

slingamn commented Jul 22, 2024

kylef Jul 25, 2024

kdumontnu Jul 25, 2024

slingamn Jul 25, 2024

fix OIDC introspection authentication #31632

fix OIDC introspection authentication #31632

Conversation

slingamn commented Jul 15, 2024 • edited Loading

slingamn commented Jul 18, 2024

delvh commented Jul 19, 2024 • edited Loading

slingamn commented Jul 19, 2024

techknowlogick commented Jul 19, 2024

lafriks commented Jul 20, 2024

slingamn commented Jul 22, 2024

kylef Jul 25, 2024

Choose a reason for hiding this comment

kdumontnu Jul 25, 2024

Choose a reason for hiding this comment

slingamn Jul 25, 2024

Choose a reason for hiding this comment

slingamn commented Jul 15, 2024 •

edited

Loading

delvh commented Jul 19, 2024 •

edited

Loading