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

DISABLE_ACCESS_TOKENS parameter for disabling access tokens added #18488

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pboguslawski
Copy link
Contributor

Access tokens are hardcoded and cannot be disabled (i.e. when
owner doesn't want this kind of authentication).

This mod introduces new DISABLE_ACCESS_TOKENS parameter in app.ini
section [security]. When disabled (default when parameter is not
present) gitea behaves as without this mod (access tokens feature
is available). When enabled, access tokens feature and its UI
elements are not avaiable.

This mod also hides those areas on Settings/Applications page
that are disabled in config and hides menu link to Applications
page if all its areas are disabled in config.

Related: #13129
Author-Change-Id: IB#1115254

Access tokens are hardcoded and cannot be disabled (i.e. when
owner doesn't want this kind of authentication).

This mod introduces new DISABLE_ACCESS_TOKENS parameter in app.ini
section [security]. When disabled (default when parameter is not
present) gitea behaves as without this mod (access tokens feature
is available). When enabled, access tokens feature and its UI
elements are not avaiable.

This mod also hides those areas on Settings/Applications page
that are disabled in config and hides menu link to Applications
page if all its areas are disabled in config.

Related: go-gitea#13129
Author-Change-Id: IB#1115254
Fixes: 2bde83b
Author-Change-Id: IB#1115254
@CirnoT
Copy link
Contributor

CirnoT commented Feb 1, 2022

What is your potential reasoning and use-case for wanting to have these disabled? Realistically, almost no one will want to have it disabled as it breaks integration of almost any external service (including all CI/CD). I find it doubtful anyone would intentionally want to gut their instance that way.

In addition, this breaks the desired use-case of tokens instead of username/password combination for Basic Auth for Git operations.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 1, 2022
"DisableAccessTokens": func() bool {
return setting.DisableAccessTokens
},
"DisableOAuth2": func() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't mention anything about OAuth2 in this PR, what gives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is your potential reasoning and use-case for wanting to have these disabled?

If someone want to disable some auth options (like tokens) - should be allowed to do it (i.e. for security reasons). Other stuff like OAuth2 has switches.

Realistically, almost no one will want to have it disabled as it breaks integration of almost any external service (including all CI/CD)

Think of systems that are isolated from all other apps and allows only authenticated user access (i.e. for security reasons).

All optional stuff in gitea should have switch to disable it. Don't try to be smarter than app owner.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find your security argument not convincing, considering that token authentication is far more secure than username/password combination for Basic Auth. Can you elaborate exactly how disabling this brings improved security over users being forced to specify (and remember) their username/password combinations in Git clients?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate exactly how disabling this brings improved security over users being forced to specify (and remember) their username/password combinations in Git clients?

Think of authentication with HTTP header using reverse proxy. In this scenario one may want to authenticate users ONLY with HTTP header from proxy and disable all other auth stuff (like ssh keys, passwords, basic auths and other shiny toys). Now its proxy job to make auth secure (SSO for example using secure auth methods).

Copy link
Contributor Author

@pboguslawski pboguslawski Feb 2, 2022

Choose a reason for hiding this comment

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

Git clients are already smart enough to use more advanced auth methods than username+password. Tokens should be left available as option of course if one needs it (i.e. external app that understands tokens only).

Copy link
Contributor

Choose a reason for hiding this comment

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

Think of authentication with HTTP header using reverse proxy. In this scenario one may want to authenticate users ONLY with HTTP header from proxy and disable all other auth stuff (like ssh keys, passwords, basic auths and other shiny toys). Now its proxy job to make auth secure (SSO for example using secure auth methods).

Assuming that Gitea can authenticate Git requests this way it would be a valid scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can.

@techknowlogick techknowlogick added this to the 1.17.0 milestone Feb 2, 2022
@lunny
Copy link
Member

lunny commented Feb 3, 2022

What is your potential reasoning and use-case for wanting to have these disabled? Realistically, almost no one will want to have it disabled as it breaks integration of almost any external service (including all CI/CD). I find it doubtful anyone would intentionally want to gut their instance that way.

In addition, this breaks the desired use-case of tokens instead of username/password combination for Basic Auth for Git operations.

I also think the reasons and scenarios are necessary.

@techknowlogick
Copy link
Member

+1 for what @CirnoT has said. We will eventually remove user/pass from git clone and this would leave users without a way to clone via HTTPS.

@pboguslawski
Copy link
Contributor Author

We will eventually remove user/pass from git clone and this would leave users without a way to clone via HTTPS.

This PR is not about basic auth. It's about allowing system owner to decide if token auth will be available or not in their gitea instance. This PR does not change default gitea behaviour - tokens will be still available by default.

All auth methods should be optional and system admin (not gitea dev) should choose what is appropriate for given system instance. Basic auth, reverse proxy, oauth... are optional now; after this mod tokens should be optional also.

@CirnoT
Copy link
Contributor

CirnoT commented Feb 3, 2022

@techknowlogick @lunny I rest my case here. Gitea can authenticate HTTP requests via reverse proxy authentication (SSO, etc.) so in this scenario tokens can be disabled and user would still not be forced to use username/password authentication (ie. they would authenticate Git credentials with their SSO provider which would pass proper authorization to Gitea).

Copy link
Contributor

@CirnoT CirnoT left a comment

Choose a reason for hiding this comment

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

While this PR does hide the UI and ability to modify tokens, it does not disable authentication with existing ones which is a major oversight.

@@ -372,6 +372,9 @@ INTERNAL_TOKEN=
;; Set to true to disable webhooks feature.
;DISABLE_WEBHOOKS = false
;;
;; Set to false to disable access tokens feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a comment here that it should be changed only if using some kind of SSO and that it might break existing integrations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd add a comment here that it should be changed only if using some kind of SSO and that it might break existing integrations.

If someone is touching such settings, should test it before in nonprod env. Describing all possible scenarios here (like SSO/noSSO, only basic auth without tokens, etc.) does not make sense IHMO.

@pboguslawski
Copy link
Contributor Author

pboguslawski commented Feb 3, 2022

While this PR does hide the UI and ability to modify tokens, it does not disable authentication with existing ones which is a major oversight.

Fixed in 3de67bf.

When DISABLE_ACCESS_TOKENS=true, existing access tokens should be disabled.

Fixes: 2bde83b
Related: go-gitea#18488 (review)
Author-Change-Id: IB#1115254
@stale
Copy link

stale bot commented Apr 17, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 17, 2022
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Apr 18, 2022
@stale stale bot removed the issue/stale label Apr 18, 2022
@lunny lunny modified the milestones: 1.17.0, 1.18.0 Jun 4, 2022
@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022
@lunny
Copy link
Member

lunny commented Feb 1, 2023

access token should not be disabled except disabled API

@lunny lunny removed this from the 1.19.0 milestone Feb 1, 2023
@denyskon
Copy link
Member

Is there a consensus that this functionality is not wanted? Then we should close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants