Prevent double-login for Git HTTP and LFS and simplify login (go-gite…

…a#15303) * Prevent double-login for Git HTTP and LFS and simplify login There are a number of inconsistencies with our current methods for logging in for git and lfs. The first is that there is a double login process. This is particularly evident in 1.13 where there are no less than 4 hash checks for basic authentication due to the previous IsPasswordSet behaviour. This duplicated code had individual inconsistencies that were not helpful and caused confusion. This PR does the following: * Remove the specific login code from the git and lfs handlers except for the lfs special bearer token * Simplify the meaning of DisableBasicAuthentication to allow Token and Oauth2 sign-in. * The removal of the specific code from git and lfs means that these both now have the same login semantics and can - if not DisableBasicAuthentication - login from external services. Further it allows Oauth2 token authentication as per our standard mechanisms. * The change in the recovery handler prevents the service from re-attempting to login - primarily because this could easily cause a further panic and it is wasteful. * add test Signed-off-by: Andrew Thornton <[email protected]> Co-authored-by: Andrew Thornton <[email protected]>
kitspace · Aug 10, 2021 · 301ec6d · 301ec6d
1 parent 6abd9dd
commit 301ec6d
Show file tree

Hide file tree

Showing 10 changed files with 288 additions and 217 deletions.
diff --git a/integrations/api_repo_lfs_locks_test.go b/integrations/api_repo_lfs_locks_test.go
@@ -44,7 +44,7 @@ func TestAPILFSLocksNotLogin(t *testing.T) {
 	resp := MakeRequest(t, req, http.StatusUnauthorized)
 	var lfsLockError api.LFSLockError
 	DecodeJSON(t, resp, &lfsLockError)
-	assert.Equal(t, "Unauthorized", lfsLockError.Message)
+	assert.Equal(t, "You must have pull access to list locks", lfsLockError.Message)
 }
 
 func TestAPILFSLocksLogged(t *testing.T) {

diff --git a/modules/auth/sso/basic.go b/modules/auth/sso/basic.go
@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/web/middleware"
 )
 
 // Ensure the struct implements the interface.
@@ -40,25 +41,30 @@ func (b *Basic) Free() error {
 // IsEnabled returns true as this plugin is enabled by default and its not possible to disable
 // it from settings.
 func (b *Basic) IsEnabled() bool {
-	return setting.Service.EnableBasicAuth
+	return true
 }
 
 // VerifyAuthData extracts and validates Basic data (username and password/token) from the
 // "Authorization" header of the request and returns the corresponding user object for that
 // name/token on successful validation.
 // Returns nil if header is empty or validation fails.
 func (b *Basic) VerifyAuthData(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *models.User {
+
+	// Basic authentication should only fire on API, Download or on Git or LFSPaths
+	if middleware.IsInternalPath(req) || !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitOrLFSPath(req) {
+		return nil
+	}
+
 	baHead := req.Header.Get("Authorization")
 	if len(baHead) == 0 {
 		return nil
 	}
 
-	auths := strings.Fields(baHead)
+	auths := strings.SplitN(baHead, " ", 2)
 	if len(auths) != 2 || (auths[0] != "Basic" && auths[0] != "basic") {
 		return nil
 	}
 
-	var u *models.User
 	uname, passwd, _ := base.BasicAuthDecode(auths[1])
 
 	// Check if username or password is a token
@@ -76,20 +82,21 @@ func (b *Basic) VerifyAuthData(req *http.Request, w http.ResponseWriter, store D
 	uid := CheckOAuthAccessToken(authToken)
 	if uid != 0 {
 		log.Trace("Basic Authorization: Valid OAuthAccessToken for user[%d]", uid)
-		var err error
-		store.GetData()["IsApiToken"] = true
 
-		u, err = models.GetUserByID(uid)
+		u, err := models.GetUserByID(uid)
 		if err != nil {
 			log.Error("GetUserByID:  %v", err)
 			return nil
 		}
+
+		store.GetData()["IsApiToken"] = true
+		return u
 	}
+
 	token, err := models.GetAccessTokenBySHA(authToken)
 	if err == nil {
 		log.Trace("Basic Authorization: Valid AccessToken for user[%d]", uid)
-
-		u, err = models.GetUserByID(token.UID)
+		u, err := models.GetUserByID(token.UID)
 		if err != nil {
 			log.Error("GetUserByID:  %v", err)
 			return nil
@@ -99,22 +106,24 @@ func (b *Basic) VerifyAuthData(req *http.Request, w http.ResponseWriter, store D
 		if err = models.UpdateAccessToken(token); err != nil {
 			log.Error("UpdateAccessToken:  %v", err)
 		}
+
+		store.GetData()["IsApiToken"] = true
+		return u
 	} else if !models.IsErrAccessTokenNotExist(err) && !models.IsErrAccessTokenEmpty(err) {
 		log.Error("GetAccessTokenBySha: %v", err)
 	}
 
-	if u == nil {
-		log.Trace("Basic Authorization: Attempting SignIn for %s", uname)
+	if !setting.Service.EnableBasicAuth {
+		return nil
+	}
 
-		u, err = models.UserSignIn(uname, passwd)
-		if err != nil {
-			if !models.IsErrUserNotExist(err) {
-				log.Error("UserSignIn: %v", err)
-			}
-			return nil
+	log.Trace("Basic Authorization: Attempting SignIn for %s", uname)
+	u, err := models.UserSignIn(uname, passwd)
+	if err != nil {
+		if !models.IsErrUserNotExist(err) {
+			log.Error("UserSignIn: %v", err)
 		}
-	} else {
-		store.GetData()["IsApiToken"] = true
+		return nil
 	}
 
 	log.Trace("Basic Authorization: Logged in user %-v", u)

diff --git a/modules/auth/sso/sso.go b/modules/auth/sso/sso.go
@@ -9,10 +9,12 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
+	"regexp"
 	"strings"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/web/middleware"
 )
 
@@ -27,9 +29,9 @@ import (
 // for users that have already signed in.
 var ssoMethods = []SingleSignOn{
 	&OAuth2{},
+	&Basic{},
 	&Session{},
 	&ReverseProxy{},
-	&Basic{},
 }
 
 // The purpose of the following three function variables is to let the linter know that
@@ -102,6 +104,19 @@ func isAttachmentDownload(req *http.Request) bool {
 	return strings.HasPrefix(req.URL.Path, "/attachments/") && req.Method == "GET"
 }
 
+var gitPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/(?:(?:git-(?:(?:upload)|(?:receive))-pack$)|(?:info/refs$)|(?:HEAD$)|(?:objects/))`)
+var lfsPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/info/lfs/`)
+
+func isGitOrLFSPath(req *http.Request) bool {
+	if gitPathRe.MatchString(req.URL.Path) {
+		return true
+	}
+	if setting.LFS.StartServer {
+		return lfsPathRe.MatchString(req.URL.Path)
+	}
+	return false
+}
+
 // handleSignIn clears existing session variables and stores new ones for the specified user object
 func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *models.User) {
 	_ = sess.Delete("openid_verified_uri")

diff --git a/modules/auth/sso/sso_test.go b/modules/auth/sso/sso_test.go
@@ -0,0 +1,124 @@
+// Copyright 2014 The Gogs Authors. All rights reserved.
+// Copyright 2019 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package sso
+
+import (
+	"net/http"
+	"testing"
+
+	"code.gitea.io/gitea/modules/setting"
+)
+
+func Test_isGitOrLFSPath(t *testing.T) {
+
+	tests := []struct {
+		path string
+
+		want bool
+	}{
+		{
+			"/owner/repo/git-upload-pack",
+			true,
+		},
+		{
+			"/owner/repo/git-receive-pack",
+			true,
+		},
+		{
+			"/owner/repo/info/refs",
+			true,
+		},
+		{
+			"/owner/repo/HEAD",
+			true,
+		},
+		{
+			"/owner/repo/objects/info/alternates",
+			true,
+		},
+		{
+			"/owner/repo/objects/info/http-alternates",
+			true,
+		},
+		{
+			"/owner/repo/objects/info/packs",
+			true,
+		},
+		{
+			"/owner/repo/objects/info/blahahsdhsdkla",
+			true,
+		},
+		{
+			"/owner/repo/objects/01/23456789abcdef0123456789abcdef01234567",
+			true,
+		},
+		{
+			"/owner/repo/objects/pack/pack-123456789012345678921234567893124567894.pack",
+			true,
+		},
+		{
+			"/owner/repo/objects/pack/pack-0123456789abcdef0123456789abcdef0123456.idx",
+			true,
+		},
+		{
+			"/owner/repo/stars",
+			false,
+		},
+		{
+			"/notowner",
+			false,
+		},
+		{
+			"/owner/repo",
+			false,
+		},
+		{
+			"/owner/repo/commit/123456789012345678921234567893124567894",
+			false,
+		},
+	}
+	lfsTests := []string{
+		"/owner/repo/info/lfs/",
+		"/owner/repo/info/lfs/objects/batch",
+		"/owner/repo/info/lfs/objects/oid/filename",
+		"/owner/repo/info/lfs/objects/oid",
+		"/owner/repo/info/lfs/objects",
+		"/owner/repo/info/lfs/verify",
+		"/owner/repo/info/lfs/locks",
+		"/owner/repo/info/lfs/locks/verify",
+		"/owner/repo/info/lfs/locks/123/unlock",
+	}
+
+	origLFSStartServer := setting.LFS.StartServer
+
+	for _, tt := range tests {
+		t.Run(tt.path, func(t *testing.T) {
+			req, _ := http.NewRequest("POST", "http://localhost"+tt.path, nil)
+			setting.LFS.StartServer = false
+			if got := isGitOrLFSPath(req); got != tt.want {
+				t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want)
+			}
+			setting.LFS.StartServer = true
+			if got := isGitOrLFSPath(req); got != tt.want {
+				t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+	for _, tt := range lfsTests {
+		t.Run(tt, func(t *testing.T) {
+			req, _ := http.NewRequest("POST", tt, nil)
+			setting.LFS.StartServer = false
+			if got := isGitOrLFSPath(req); got != setting.LFS.StartServer {
+				t.Errorf("isGitOrLFSPath(%q) = %v, want %v, %v", tt, got, setting.LFS.StartServer, gitPathRe.MatchString(tt))
+			}
+			setting.LFS.StartServer = true
+			if got := isGitOrLFSPath(req); got != setting.LFS.StartServer {
+				t.Errorf("isGitOrLFSPath(%q) = %v, want %v", tt, got, setting.LFS.StartServer)
+			}
+		})
+	}
+	setting.LFS.StartServer = origLFSStartServer
+}
diff --git a/modules/context/context.go b/modules/context/context.go
@@ -683,6 +683,9 @@ func Contexter() func(next http.Handler) http.Handler {
 			} else {
 				ctx.Data["SignedUserID"] = int64(0)
 				ctx.Data["SignedUserName"] = ""
+
+				// ensure the session uid is deleted
+				_ = ctx.Session.Delete("uid")
 			}
 
 			ctx.Resp.Header().Set(`X-Frame-Options`, `SAMEORIGIN`)

diff --git a/routers/private/internal.go b/routers/private/internal.go
@@ -23,7 +23,7 @@ import (
 func CheckInternalToken(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		tokens := req.Header.Get("Authorization")
-		fields := strings.Fields(tokens)
+		fields := strings.SplitN(tokens, " ", 2)
 		if len(fields) != 2 || fields[0] != "Bearer" || fields[1] != setting.InternalToken {
 			log.Debug("Forbidden attempt to access internal url: Authorization header: %s", tokens)
 			http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)