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.

Sign up for GitHub

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

Only allow token authentication with 2FA enabled #2184

Merged
merged 6 commits into from
Jul 26, 2017
Merged

Only allow token authentication with 2FA enabled #2184

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7673b8b
Don't allow for plain username/password authentication when 2FA is en…
Jul 19, 2017
02a101e
Removed debugging statement
Jul 19, 2017
6ce636f
Don't assume a token belongs to a given user, handle two-factor error…
Jul 20, 2017
e6484d5
Simplified user/token matching, refactored error handling for two-fac…
Jul 20, 2017
f6857ec
Change authentication response to avoid bruteforcing
Jul 20, 2017
9a14c6d
Add TODO item as a comment for changing the response for security pur…
Jul 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions routers/repo/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,22 @@ func HTTP(ctx *context.Context) {
ctx.Handle(http.StatusInternalServerError, "UserSignIn error: %v", err)
return
}
}

if authUser == nil {
authUser, err = models.GetUserByName(authUsername)

if err != nil {
if models.IsErrUserNotExist(err) {
ctx.HandleText(http.StatusUnauthorized, "invalid credentials")
} else {
ctx.Handle(http.StatusInternalServerError, "GetUserByName", err)
}
return
}

// Assume username now is a token.
token, err := models.GetAccessTokenBySHA(authUsername)
// Assume password is a token.
token, err := models.GetAccessTokenBySHA(authPasswd)
Copy link
Member

Choose a reason for hiding this comment

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

You need to also check if user models.GetUserByName(authUsername) returns correct user and validate that returned user.ID is equal to token.UID

if err != nil {
if models.IsErrAccessTokenNotExist(err) || models.IsErrAccessTokenEmpty(err) {
ctx.HandleText(http.StatusUnauthorized, "invalid token")
Expand All @@ -167,13 +180,25 @@ func HTTP(ctx *context.Context) {
}
return
}

if authUser.ID != token.UID {
ctx.HandleText(http.StatusUnauthorized, "invalid credentials")
return
}

token.Updated = time.Now()
if err = models.UpdateAccessToken(token); err != nil {
ctx.Handle(http.StatusInternalServerError, "UpdateAccessToken", err)
}
authUser, err = models.GetUserByID(token.UID)
if err != nil {
ctx.Handle(http.StatusInternalServerError, "GetUserByID", err)

} else {
_, err = models.GetTwoFactorByUID(authUser.ID)

if err == nil {
Copy link
Member

@lafriks lafriks Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You need to also check that if !models.IsErrTwoFactorNotEnrolled(err) and do ctx.Handle(http.StatusInternalServerError, "IsErrTwoFactorNotEnrolled", err) + return otherwise if there is some kind of unexpected error you will let user in and it will bypass 2fa

ctx.HandleText(http.StatusUnauthorized, "Users with two-factor authentication enabled cannot perform HTTP/HTTPS operations via plain username and password. Please create and use a personal access token on the user settings page")
return
} else if !models.IsErrTwoFactorNotEnrolled(err) {
ctx.Handle(http.StatusInternalServerError, "IsErrTwoFactorNotEnrolled", err)
return
}
}
Expand Down