Deprecate query string auth tokens #28390

jackHay22 · 2023-12-07T19:15:33Z

Changes

Add deprecation warning to Token and AccessToken authentication methods in swagger.

Add deprecation warning header to API response. Example:

HTTP/1.1 200 OK
...
Warning: token and access_token API authentication is deprecated
...

Add setting DISABLE_QUERY_AUTH_TOKEN to reject query string auth tokens entirely. Default is false

Next steps

DISABLE_QUERY_AUTH_TOKEN should be true in a subsequent release and the methods should be removed in swagger
DISABLE_QUERY_AUTH_TOKEN should be removed and the implementation of the auth methods in question should be removed

Open questions

Should there be further changes to the swagger documentation? Deprecation is not yet supported for security definitions (coming in OpenAPI Spec version 3.2.0)
Should the API router logger sanitize urls that use token or access_token? (This is obviously an insufficient solution on its own)

kdumontnu

Just some minor suggestions

modules/setting/security.go

routers/api/v1/api.go

kdumontnu · 2023-12-07T21:47:17Z

This PR aims to solve a security vulnerability in which auth tokens in URL query parameters are leaked to middleware applications that log URL information, including access logs, browser history, and analytics services (such as Google Analytics).

More information here: https://owasp.org/www-community/vulnerabilities/Information_exposure_through_query_strings_in_url

It is a major functional change, as token auth is likely the most common method for authentication. However, a migration to token in headers should be fairly straightforward for users.

The target is to provide deprecation warnings for users starting in 1.22 (along with a gitea config setting that can be used to disable the setting immediately). The default setting will be to disable query-based authentication in 1.23, and then remove the setting in 1.24.

custom/conf/app.example.ini

modules/setting/security.go

routers/api/v1/api.go

delvh · 2023-12-07T22:24:47Z

Should we perhaps backport this to 1.21?
Then users have more time to see the pending change, and currently nothing changes.

kdumontnu · 2023-12-07T22:33:15Z

Should we perhaps backport this to 1.21? Then users have more time to see the pending change, and currently nothing changes.

Yeah, I think that's a great idea.

KN4CK3R · 2023-12-08T17:57:27Z

I can create a PR which removes the token usage from the integration tests. There we use the parameter a lot.

Co-authored-by: delvh <dev.lh@web.de>

lunny · 2023-12-11T01:21:13Z

Maybe we can have a warning, the support will be removed at some version, like 1.23. We can removed the support entirely at that time but not not opt-out.

delvh · 2023-12-11T01:26:34Z

Doesn't make any difference.
I think this is already included in the message to some extent.

kdumontnu

👍

If lunny means a warning message at startup I think that's a good idea too - the more warnings the better

@jackHay22

Backport #28390 by @jackHay22 ## Changes - Add deprecation warning to `Token` and `AccessToken` authentication methods in swagger. - Add deprecation warning header to API response. Example: ``` HTTP/1.1 200 OK ... Warning: token and access_token API authentication is deprecated ... ``` - Add setting `DISABLE_QUERY_AUTH_TOKEN` to reject query string auth tokens entirely. Default is `false` ## Next steps - `DISABLE_QUERY_AUTH_TOKEN` should be true in a subsequent release and the methods should be removed in swagger - `DISABLE_QUERY_AUTH_TOKEN` should be removed and the implementation of the auth methods in question should be removed ## Open questions - Should there be further changes to the swagger documentation? Deprecation is not yet supported for security definitions (coming in [OpenAPI Spec version 3.2.0](OAI/OpenAPI-Specification#2506)) - Should the API router logger sanitize urls that use `token` or `access_token`? (This is obviously an insufficient solution on its own) Co-authored-by: Jack Hay <jack@allspice.io> Co-authored-by: delvh <dev.lh@web.de>

* giteaofficial/main: [skip ci] Updated translations via Crowdin Fix possible nil pointer access (go-gitea#28428) Don't show unnecessary citation JS error on UI (go-gitea#28433) Do some missing checks (go-gitea#28423) Deprecate query string auth tokens (go-gitea#28390)

wolfogre · 2023-12-15T09:27:33Z

Maybe we shoud update the configuration cheat sheet? I noticed the warning log and tried to check more on https://docs.gitea.com/administration/config-cheat-sheet, but I found nothing and I had to read the code.

kdumontnu · 2023-12-15T15:41:07Z

Maybe we shoud update the configuration cheat sheet? I noticed the warning log and tried to check more on https://docs.gitea.com/administration/config-cheat-sheet, but I found nothing and I had to read the code.

Yeah, that was an oversite. I can patch that.

As described [here](#28390 (comment)).

As described [here](go-gitea#28390 (comment)).

@kdumontnu

Backport #28485 by @kdumontnu As described [here](#28390 (comment)). Co-authored-by: Kyle D <kdumontnu@gmail.com>

pat-s · 2023-12-21T07:52:12Z

Just for ref: this caused an issue in the helm-chart: https://gitea.com/gitea/helm-chart/pulls/590

Related #28390

Related go-gitea#28390

… defined (#28783) So we don't warn on default behavior - Fixes #28758 - Follows #28390 Signed-off-by: Yarden Shoham <git@yardenshoham.com>

## Changes - Add deprecation warning to `Token` and `AccessToken` authentication methods in swagger. - Add deprecation warning header to API response. Example: ``` HTTP/1.1 200 OK ... Warning: token and access_token API authentication is deprecated ... ``` - Add setting `DISABLE_QUERY_AUTH_TOKEN` to reject query string auth tokens entirely. Default is `false` ## Next steps - `DISABLE_QUERY_AUTH_TOKEN` should be true in a subsequent release and the methods should be removed in swagger - `DISABLE_QUERY_AUTH_TOKEN` should be removed and the implementation of the auth methods in question should be removed ## Open questions - Should there be further changes to the swagger documentation? Deprecation is not yet supported for security definitions (coming in [OpenAPI Spec version 3.2.0](OAI/OpenAPI-Specification#2506)) - Should the API router logger sanitize urls that use `token` or `access_token`? (This is obviously an insufficient solution on its own) --------- Co-authored-by: delvh <dev.lh@web.de>

As described [here](go-gitea#28390 (comment)).

Related go-gitea#28390

… defined (go-gitea#28783) So we don't warn on default behavior - Fixes go-gitea#28758 - Follows go-gitea#28390 Signed-off-by: Yarden Shoham <git@yardenshoham.com>

@yardenshoham

… defined (#28783) (#28868) Backport #28783 by @yardenshoham So we don't warn on default behavior - Fixes #28758 - Follows #28390 Signed-off-by: Yarden Shoham <git@yardenshoham.com> Co-authored-by: Yarden Shoham <git@yardenshoham.com>

### Description of the change With go-gitea/gitea#28390, Gitea 1.21.2 introduced warning log output within the result of `gitea admin <subcommand>` and therefore affects the current provisioning script. That script previously assumed a clean result set and was therefore doomed to fail at _some_ point. This introduces output sanitizing to trim such logs above the actual result table. ### Applicable issues - fixes #589 ### Additional information The non-sanitized output were only an issue for admin account provisioning, and only when the username matched one of these words (in case of #589 it was `gitea`): ```text .../setting/security.go:168:loadSecurityFrom() [W] Enabling Query API Auth tokens is not recommended. DISABLE_QUERY_AUTH_TOKEN will default to true in gitea 1.23 and will be removed in gitea 1.24. ``` LDAP and OAuth sources were not affected by this particular log line, but also processed non-sanitized result sets. Changing their code is a precaution. Reviewed-on: https://gitea.com/gitea/helm-chart/pulls/590 Reviewed-by: pat-s <pat-s@noreply.gitea.com> Co-authored-by: justusbunsi <sk.bunsenbrenner@gmail.com> Co-committed-by: justusbunsi <sk.bunsenbrenner@gmail.com>

## Changes - Add deprecation warning to `Token` and `AccessToken` authentication methods in swagger. - Add deprecation warning header to API response. Example: ``` HTTP/1.1 200 OK ... Warning: token and access_token API authentication is deprecated ... ``` - Add setting `DISABLE_QUERY_AUTH_TOKEN` to reject query string auth tokens entirely. Default is `false` ## Next steps - `DISABLE_QUERY_AUTH_TOKEN` should be true in a subsequent release and the methods should be removed in swagger - `DISABLE_QUERY_AUTH_TOKEN` should be removed and the implementation of the auth methods in question should be removed ## Open questions - Should there be further changes to the swagger documentation? Deprecation is not yet supported for security definitions (coming in [OpenAPI Spec version 3.2.0](OAI/OpenAPI-Specification#2506)) - Should the API router logger sanitize urls that use `token` or `access_token`? (This is obviously an insufficient solution on its own) --------- Co-authored-by: delvh <dev.lh@web.de>

As described [here](go-gitea#28390 (comment)).

Related go-gitea#28390

… defined (go-gitea#28783) So we don't warn on default behavior - Fixes go-gitea#28758 - Follows go-gitea#28390 Signed-off-by: Yarden Shoham <git@yardenshoham.com>

jackHay22 added 2 commits December 6, 2023 10:27

add setting to disable query auth

7a91838

add api deprecation warning

89615f3

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 7, 2023

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 7, 2023

github-actions bot added the modifies/api This PR adds API routes or modifies them label Dec 7, 2023

kdumontnu suggested changes Dec 7, 2023

View reviewed changes

modules/setting/security.go Show resolved Hide resolved

routers/api/v1/api.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 Dec 7, 2023

delvh reviewed Dec 7, 2023

View reviewed changes

custom/conf/app.example.ini Show resolved Hide resolved

modules/setting/security.go Show resolved Hide resolved

routers/api/v1/api.go Outdated Show resolved Hide resolved

routers/api/v1/api.go Outdated Show resolved Hide resolved

delvh added topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! backport/v1.21 This PR should be backported to Gitea 1.21 labels Dec 7, 2023

Apply suggestions from code review

acd7a63

Co-authored-by: delvh <dev.lh@web.de>

jackHay22 requested review from techknowlogick, delvh and kdumontnu December 8, 2023 18:57

swagger generate

1a43f7c

delvh approved these changes Dec 8, 2023

View reviewed changes

techknowlogick approved these changes Dec 10, 2023

View reviewed changes

delvh changed the title ~~Deprecate/disable query string auth tokens~~ Deprecate query string auth tokens Dec 10, 2023

kdumontnu approved these changes Dec 11, 2023

View reviewed changes

GiteaBot removed the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Dec 11, 2023

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 12, 2023

KN4CK3R mentioned this pull request Dec 15, 2023

Convert to url auth to header auth in tests #28484

Merged

kdumontnu mentioned this pull request Dec 15, 2023

Update docs for DISABLE_QUERY_AUTH_TOKEN #28485

Merged

GiteaBot mentioned this pull request Dec 16, 2023

Update docs for DISABLE_QUERY_AUTH_TOKEN (#28485) #28488

Merged

techknowlogick pushed a commit that referenced this pull request Dec 16, 2023

Update docs for DISABLE_QUERY_AUTH_TOKEN (#28485)

b8490d5

As described [here](#28390 (comment)).

GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Dec 16, 2023

Update docs for DISABLE_QUERY_AUTH_TOKEN (go-gitea#28485)

7087276

As described [here](go-gitea#28390 (comment)).

techknowlogick pushed a commit that referenced this pull request Dec 16, 2023

Update docs for DISABLE_QUERY_AUTH_TOKEN (#28485) (#28488)

2c2e008

Backport #28485 by @kdumontnu As described [here](#28390 (comment)). Co-authored-by: Kyle D <kdumontnu@gmail.com>

lunny pushed a commit that referenced this pull request Dec 22, 2023

Convert to url auth to header auth in tests (#28484)

838db2f

Related #28390

techknowlogick pushed a commit to techknowlogick/gitea that referenced this pull request Dec 23, 2023

Convert to url auth to header auth in tests (go-gitea#28484)

de0585a

Related go-gitea#28390

yardenshoham added the type/deprecation Previously provided functionality is removed label Jan 14, 2024

yardenshoham mentioned this pull request Jan 14, 2024

Warn that DISABLE_QUERY_AUTH_TOKEN is false only if it's explicitly defined #28783

Merged

KN4CK3R pushed a commit that referenced this pull request Jan 14, 2024

Warn that DISABLE_QUERY_AUTH_TOKEN is false only if it's explicitly…

5a7bacb

… defined (#28783) So we don't warn on default behavior - Fixes #28758 - Follows #28390 Signed-off-by: Yarden Shoham <git@yardenshoham.com>

fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024

Update docs for DISABLE_QUERY_AUTH_TOKEN (go-gitea#28485)

d936bb6

As described [here](go-gitea#28390 (comment)).

fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024

Convert to url auth to header auth in tests (go-gitea#28484)

d96075e

Related go-gitea#28390

GiteaBot mentioned this pull request Jan 20, 2024

Warn that DISABLE_QUERY_AUTH_TOKEN is false only if it's explicitly defined (#28783) #28868

Merged

silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024

Update docs for DISABLE_QUERY_AUTH_TOKEN (go-gitea#28485)

2a1dc23

As described [here](go-gitea#28390 (comment)).

silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024

Convert to url auth to header auth in tests (go-gitea#28484)

bcb1571

Related go-gitea#28390

github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate query string auth tokens #28390

Deprecate query string auth tokens #28390

jackHay22 commented Dec 7, 2023

kdumontnu left a comment

kdumontnu commented Dec 7, 2023

delvh commented Dec 7, 2023

kdumontnu commented Dec 7, 2023

KN4CK3R commented Dec 8, 2023

lunny commented Dec 11, 2023

delvh commented Dec 11, 2023

kdumontnu left a comment •

edited

Loading

wolfogre commented Dec 15, 2023

kdumontnu commented Dec 15, 2023

pat-s commented Dec 21, 2023

Deprecate query string auth tokens #28390

Deprecate query string auth tokens #28390

Conversation

jackHay22 commented Dec 7, 2023

Changes

Next steps

Open questions

kdumontnu left a comment

Choose a reason for hiding this comment

kdumontnu commented Dec 7, 2023

delvh commented Dec 7, 2023

kdumontnu commented Dec 7, 2023

KN4CK3R commented Dec 8, 2023

lunny commented Dec 11, 2023

delvh commented Dec 11, 2023

kdumontnu left a comment • edited Loading

Choose a reason for hiding this comment

wolfogre commented Dec 15, 2023

kdumontnu commented Dec 15, 2023

pat-s commented Dec 21, 2023

kdumontnu left a comment •

edited

Loading