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.

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

Minimal OpenID Connect implementation #14139

Merged
merged 5 commits into from
Jan 1, 2021

Conversation

ojohnny
Copy link
Contributor

@ojohnny ojohnny commented Dec 24, 2020

This is "minimal" in the sense that only the Authorization Code Flow
from OpenID Connect Core is implemented. No discovery, no configuration
endpoint, and no user scope management.

OpenID Connect is an extension to the (already implemented) OAuth 2.0
protocol, and essentially an id_token JWT is added to the access token
endpoint response when using the Authorization Code Flow. I also added
support for the "nonce" field since it is required to be used in the
id_token if the client decides to include it in its initial request.

In order to enable this extension an OAuth 2.0 scope containing
"openid" is needed. Other OAuth 2.0 requests should not be impacted by
this change.

This minimal implementation is enough to enable single sign-on (SSO)
for other sites, e.g. by using something like mod_auth_openidc to
only allow access to a CI server if a user has logged into Gitea.

Fixes: #1310

@lafriks
Copy link
Member

lafriks commented Dec 24, 2020

Please run make fmt

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 24, 2020
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Dec 24, 2020
@lafriks lafriks added this to the 1.14.0 milestone Dec 24, 2020
@zeripath
Copy link
Contributor

… and make lint-backend

@ojohnny
Copy link
Contributor Author

ojohnny commented Dec 24, 2020

Please run make fmt

Was there something in particular you had in mind? I ran this a lot of times before pushing, and even now I don't get any more corrections when running this.

… and make lint-backend

I accidentally made some mistakes here in the "final cleanup" before pushing though...sorry for the noise.

models/migrations/v163.go Outdated Show resolved Hide resolved
@ojohnny ojohnny force-pushed the oidc-core-implementation branch 3 times, most recently from 6110e62 to a43926f Compare December 26, 2020 08:06
models/oauth2_application.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Dec 26, 2020

Codecov Report

Merging #14139 (e8a0e43) into master (c9b9b46) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14139      +/-   ##
==========================================
- Coverage   42.01%   42.00%   -0.01%     
==========================================
  Files         733      734       +1     
  Lines       78715    78770      +55     
==========================================
+ Hits        33073    33090      +17     
- Misses      40212    40243      +31     
- Partials     5430     5437       +7     
Impacted Files Coverage Δ
models/migrations/migrations.go 2.44% <ø> (ø)
models/migrations/v164.go 0.00% <0.00%> (ø)
modules/auth/user_form.go 39.65% <ø> (ø)
routers/user/oauth.go 43.32% <51.42%> (+0.51%) ⬆️
models/oauth2_application.go 67.98% <61.90%> (-0.87%) ⬇️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/git/repo_base_nogogit.go 63.63% <0.00%> (-9.10%) ⬇️
modules/git/utils.go 77.77% <0.00%> (-2.78%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.54%) ⬇️
models/gpg_key.go 53.33% <0.00%> (-0.58%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9b9b46...e8a0e43. Read the comment docs.

@ojohnny
Copy link
Contributor Author

ojohnny commented Dec 27, 2020

I think this PR will resolve #1310, perhaps I should have written that in the description box? (it seems I can't manually link issues to this PR)

@silverwind
Copy link
Member

You can just amend the commit message and put a Fixes: <link|id> into the commit message body and it will auto-close on merge.

@ojohnny ojohnny force-pushed the oidc-core-implementation branch 2 times, most recently from 16f93e0 to 962e30f Compare December 27, 2020 14:11
This is "minimal" in the sense that only the Authorization Code Flow
from OpenID Connect Core is implemented.  No discovery, no configuration
endpoint, and no user scope management.

OpenID Connect is an extension to the (already implemented) OAuth 2.0
protocol, and essentially an `id_token` JWT is added to the access token
endpoint response when using the Authorization Code Flow.  I also added
support for the "nonce" field since it is required to be used in the
id_token if the client decides to include it in its initial request.

In order to enable this extension an OAuth 2.0 scope containing
"openid" is needed. Other OAuth 2.0 requests should not be impacted by
this change.

This minimal implementation is enough to enable single sign-on (SSO)
for other sites, e.g. by using something like `mod_auth_openidc` to
only allow access to a CI server if a user has logged into Gitea.

Fixes: go-gitea#1310
@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 Dec 27, 2020
@ojohnny
Copy link
Contributor Author

ojohnny commented Dec 31, 2020

@lunny, @zeripath, @6543: Am I supposed to do more with this? Should I keep rebasing this PR onto the master branch, or should I simply wait for new activity from anyone else?

@6543
Copy link
Member

6543 commented Dec 31, 2020

@ojohnny now you only can wait until two of us had time to review, you can remind us by hitting the "update branch" button, but thats all

@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 Jan 1, 2021
@lunny lunny added type/changelog Adds the changelog for a new Gitea version type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed type/enhancement An improvement of existing functionality labels Jan 1, 2021
@lunny
Copy link
Member

lunny commented Jan 1, 2021

@lafriks I'm afraid that this should be a feature since it implemented an OpenID Provider.

@zeripath
Copy link
Contributor

zeripath commented Jan 1, 2021

This looks correct I just was looking for some way to check against a spec - but couldn't find the spec easily

@lunny lunny merged commit a07e67d into go-gitea:master Jan 1, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitea as an OpenID-2.0 provider
9 participants