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

In basic auth check for tokens before call UserSignIn #5725

Merged
merged 5 commits into from Feb 12, 2019

Conversation

@manuelluis
Copy link
Contributor

commented Jan 14, 2019

With this change we fix #5701

@codecov-io

This comment has been minimized.

Copy link

commented Jan 14, 2019

Codecov Report

Merging #5725 into master will increase coverage by 0.01%.
The diff coverage is 34.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5725      +/-   ##
==========================================
+ Coverage   37.76%   37.77%   +0.01%     
==========================================
  Files         325      325              
  Lines       47650    47686      +36     
==========================================
+ Hits        17994    18014      +20     
- Misses      27062    27074      +12     
- Partials     2594     2598       +4
Impacted Files Coverage Δ
modules/auth/auth.go 50.67% <34.14%> (-2.81%) ⬇️
routers/repo/http.go 43.39% <34.37%> (+2.58%) ⬆️
models/repo_list.go 63.29% <0%> (-1.27%) ⬇️

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 6868378...ace7b0a. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 label Jan 14, 2019

@zeripath
Copy link
Contributor

left a comment

Heya, thanks for your PR.

Changing login code is always a bit worrisome as it's an area fraught with consequences.

Your proposed login scheme is somewhat concerning to me. Is this used elsewhere? (either in Gitea or elsewhere?) I really don't like the idea of overloading the password field in this way, or of putting tokens in to the username. (Too many places will log usernames.)

If we have to overload fields - which I guess we have to because git won't let us add other fields. My suggestion would be to instead overload the username field with the type of login plus a separator not allowed in usernames on Gitea (likely +) followed by the username. The password field could then be kept for secrets.

If I'm being stupid and this is already in Gitea and you're just fixing a bug please tell me and I'll look again.

I do think this is a good idea though.

@manuelluis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

@zeripath, I think there is a bug. #5701

The {USER}:{TOKEN} and {TOKEN}:x-oauth-basic are already using for cloning repos, I'm not adding new authentication schemes.

When you use the token to clone a repo, actually gitea check first the user, twice, with UserSignIn. If the user is a gitea user (not an external user) this is not a problem, but if you are validating user to LDAP, for example, you are validating the tokens to LDAP: you can block your user since you are sending the username and the token as the password (not your user passwd) or you can ex-filtrate the token if you use the token as the username and "x-oauth-basic" as the password.

For every clone you are doing 4 user validations, it's very easy to block or disable the user in the LDAP.

In routers/repo/http.go I only change the order of the validation, first I check if is a token and then the password of the user.

In modules/auth/auth.go, SignedInUser is call in the middelware Contexter for every request. I didn't find an easy way to know if we are cloning a repo. There is already a check for the api that is very simple (checks if the path begin with "/api/") but detect if the path is a route for Git smart HTTP will be more complicated.
I added the check for the token in the basic auth. This is new since that type of auth is not permitted before: use {USER}:{TOKEN} or {TOKEN}:x-oauth-basic as basic auth.

With the Git smart HTTP requests there is a problem of double validation that is not solve in this pull.

@lafriks lafriks added this to the 1.8.0 milestone Jan 21, 2019

@lafriks

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Thanks for PR! This has been troubling me for some time on one instance with LDAP :)

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Feb 12, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Feb 12, 2019

@lafriks

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Make lgtm work

@lafriks lafriks merged commit fc038ca into go-gitea:master Feb 12, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@lafriks

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Please backport this to release/v1.7 branch

lafriks added a commit to lafriks-fork/gitea that referenced this pull request Feb 15, 2019

In basic auth check for tokens before call UserSignIn (go-gitea#5725)
* Check first if user/password is a token

* In basic auth check if user/password is a token

* Remove unnecessary else statement

* Changes of fmt

lafriks added a commit that referenced this pull request Feb 15, 2019

In basic auth check for tokens before call UserSignIn (#5725) (#6083)
* Check first if user/password is a token

* In basic auth check if user/password is a token

* Remove unnecessary else statement

* Changes of fmt

Mikescher added a commit to Mikescher/gitea that referenced this pull request Mar 20, 2019

In basic auth check for tokens before call UserSignIn (go-gitea#5725)
* Check first if user/password is a token

* In basic auth check if user/password is a token

* Remove unnecessary else statement

* Changes of fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.