Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jul 26, 2024
Merged

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

merged 3 commits into from
Jul 26, 2024

Conversation

slingamn
Copy link
Contributor

@kylef pointed out on #31632 that RFC7617 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:

gitea/services/auth/basic.go

Lines 55 to 58 in 930ca92

auths := strings.SplitN(baHead, " ", 2)
if len(auths) != 2 || (strings.ToLower(auths[0]) != "basic") {
return nil, nil
}

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 26, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 26, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jul 26, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 26, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 26, 2024
@techknowlogick techknowlogick enabled auto-merge (squash) July 26, 2024 19:22
@techknowlogick techknowlogick added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Jul 26, 2024
@techknowlogick techknowlogick merged commit e1cf760 into go-gitea:main Jul 26, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jul 26, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 26, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 29, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
5481812 
* giteaofficial/main:
  Make GetRepositoryByName more safer (go-gitea#31712)
  [skip ci] Updated licenses and gitignores
  Run `go-install` in `deps-tools` in parallel (go-gitea#31711)
  Hide the "Details" link of commit status when the user cannot access actions (go-gitea#30156)
  Enable `no-jquery/no-parse-html-literal` and fix violation (go-gitea#31684)
  [skip ci] Updated translations via Crowdin
  OIDC: case-insensitive comparison for auth scheme `Basic` (go-gitea#31706)
  Support `pull_request_target` event for commit status (go-gitea#31703)
  Add types to fetch,toast,bootstrap,svg (go-gitea#31627)
  Run `detectWebAuthnSupport` only if necessary (go-gitea#31691)
  add `username` to OIDC introspection response (go-gitea#31688)
  Add return type to GetRawFileOrLFS and GetRawFile (go-gitea#31680)
  Support delete user email in admin panel (go-gitea#31690)
  Use GetDisplayName() instead of DisplayName() to generate rss feeds (go-gitea#31687)
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Aug 8, 2024
@slingamn
cherry-pick OIDC changes from gitea (#4724)
878c236 
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.

<!--start release-notes-assistant-->

## Draft release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Breaking features
  - [PR](https://codeberg.org/forgejo/forgejo/pulls/4724): <!--number 4724 --><!--line 0 --><!--description T0lEQyBpbnRlZ3JhdGlvbnMgdGhhdCBQT1NUIHRvIGAvbG9naW4vb2F1dGgvaW50cm9zcGVjdGAgd2l0aG91dCBzZW5kaW5nIEhUVFAgYmFzaWMgYXV0aGVudGljYXRpb24gd2lsbCBub3cgZmFpbCB3aXRoIGEgNDAxIEhUVFAgVW5hdXRob3JpemVkIGVycm9yLiBUbyBmaXggdGhlIGVycm9yLCB0aGUgY2xpZW50IG11c3QgYmVnaW4gc2VuZGluZyBIVFRQIGJhc2ljIGF1dGhlbnRpY2F0aW9uIHdpdGggYSB2YWxpZCBjbGllbnQgSUQgYW5kIHNlY3JldC4gVGhpcyBlbmRwb2ludCB3YXMgcHJldmlvdXNseSBhdXRoZW50aWNhdGVkIHZpYSB0aGUgaW50cm9zcGVjdGlvbiB0b2tlbiBpdHNlbGYsIHdoaWNoIGlzIGxlc3Mgc2VjdXJlLg==-->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.<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4724
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Shivaram Lingamneni <slingamn@cs.stanford.edu>
Co-committed-by: Shivaram Lingamneni <slingamn@cs.stanford.edu>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wolfogre wolfogre wolfogre approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/authentication type/bug
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.
5 participants
@slingamn @silverwind @techknowlogick @wolfogre @GiteaBot