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

Preconfigure OAuth Git credential helpers #25653

Closed
wants to merge 1 commit into from

Conversation

hickford
Copy link
Contributor

@hickford hickford commented Jul 3, 2023

OAuth Git credential helpers are universally useful so preregister on every instance. https://git-scm.com/doc/credential-helpers

Fixes #25189

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 3, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 3, 2023
@hickford hickford changed the title preconfigure git-credential-oauth preconfigure oauth application git-credential-oauth Jul 3, 2023
@hickford hickford force-pushed the git-credential-oauth branch 6 times, most recently from fe893d8 to a1854e5 Compare July 9, 2023 21:04
@hickford
Copy link
Contributor Author

hickford commented Jul 9, 2023

Tested, working

@hickford hickford changed the title preconfigure oauth application git-credential-oauth Preconfigure OAuth Git credential helpers Jul 9, 2023
@hickford hickford marked this pull request as ready for review July 9, 2023 21:08
@hickford hickford force-pushed the git-credential-oauth branch 3 times, most recently from 2080741 to 731c2ca Compare July 9, 2023 21:48
@denyskon
Copy link
Member

I'm not a fan of that approach, because even if the admin explicitly deletes that OAuth application, it reappears on restart. I don't think that's wanted behavior.

How about the following idea:

  1. Add a (prechecked?) checkbox on installation page if OAuth applications for git credential helpers should be created
  2. Create them directly after install
  3. Add a gitea admin subcommand to create the OAuth applications for existing instances or if they were deleted previously

@denyskon denyskon added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jul 10, 2023
@lunny
Copy link
Member

lunny commented Jul 11, 2023

Or we can have a configuration to enable/disable the feature.

@denyskon
Copy link
Member

The problem as it seems to me is that our system of OAuth applications isn't made for preconfigured ones.

If we make a configuration option for this, we'd have to decide how to handle if something changes manually, for example an admin deletes one of the predefined OAuth apps while keeping the configuration parameter. This would probably result in the same thing I described in my previous comment - deleted OAuth apps reappearing again.

What we could do is creating another field for OAuth apps where we set whether they are visible are hidden in the UI.

I still think the best way to do it though would be a checkbox on the installation page, and then we maybe could add a button in the OAuth apps UI to recreate those OAuth apps if they were deleted.

@hickford
Copy link
Contributor Author

hickford commented Jul 12, 2023

even if the admin explicitly deletes that OAuth application, it reappears on restart. I don't think that's wanted behavior.

I agree, that's unexpected.

add a button in the OAuth apps UI to recreate those OAuth apps if they were deleted

I can't think why an admin would want to delete these Git credential helper OAuth apps (or later recreate them). If admin wants to disable HTTPS authentication, they can set existing configuration DISABLE_HTTP_GIT. Deleting the apps wouldn't actually block Git Credential Manager or git-credential-oauth, just break their default configuration, and force users to do manual configuration. To block them completely, admin would have to disable OAuth entirely with oauth.ENABLE = false.

Security is strong -- Gitea requires individual users consent to authorize the apps on each authentication (assuming #25061) https://imgur.com/a/7RRUPES . The applications are registered with loopback redirect URI 127.0.0.1 , so unlike web apps, there's no risk of domain hijacking. They are public clients, not confidential clients, so there's no risk of secret leakage because they don't have secrets.

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2023
@hickford
Copy link
Contributor Author

if the admin explicitly deletes that OAuth application, it reappears on restart

I've added a config option to prevent the applications being recreated, though I can't think when this would be useful.

@lunny lunny added this to the 1.21.0 milestone Jul 27, 2023
if setting.OAuth2.GitCredentialHelpers {
// the following Git credential helpers are universally useful
// https://git-scm.com/doc/credential-helpers
_ = db.Insert(ctx, []OAuth2Application{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about add a subcommand like ./gitea manager init-git-credential to insert this record into db?

@denyskon
Copy link
Member

denyskon commented Aug 2, 2023

@hickford I proposed another solution in #26291. I'll close this PR for now, feel free to reopen if you don't like my proposal.

@denyskon denyskon closed this Aug 2, 2023
@GiteaBot GiteaBot removed this from the 1.21.0 milestone Aug 2, 2023
KN4CK3R pushed a commit that referenced this pull request Aug 9, 2023
This PR is an extended implementation of #25189 and builds upon the
proposal by @hickford in #25653, utilizing some ideas proposed
internally by @wxiaoguang.

Mainly, this PR consists of a mechanism to pre-register OAuth2
applications on startup, which can be enabled or disabled by modifying
the `[oauth2].DEFAULT_APPLICATIONS` parameter in app.ini. The OAuth2
applications registered this way are being marked as "locked" and
neither be deleted nor edited over UI to prevent confusing/unexpected
behavior. Instead, they're being removed if no longer enabled in config.


![grafik](https://github.com/go-gitea/gitea/assets/47871822/81a78b1c-4b68-40a7-9e99-c272ebb8f62e)

The implemented mechanism can also be used to pre-register other OAuth2
applications in the future, if wanted.

Co-authored-by: hickford <mirth.hickford@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

---------

Co-authored-by: M Hickford <mirth.hickford@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/authentication 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.

Preconfigure Git Credential Manager as instance-wide OAuth application
6 participants