From 9e842c8a722eb1db50cfbdbe7146b67d3670052f Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Sat, 27 Jan 2018 17:48:15 +0100 Subject: [PATCH] Fix SSH auth lfs locks (#3152) * Fix SSH auth LFS locks * Activate SSH/lock test * Remove debug * Follow @lunny recommendation for AfterLoad method --- cmd/serv.go | 8 +- integrations/api_repo_lfs_locks_test.go | 8 +- integrations/git_test.go | 4 +- models/error.go | 19 ++-- models/lfs_lock.go | 55 +++++----- modules/lfs/locks.go | 78 +++++++------- modules/lfs/server.go | 133 ++++++++++++------------ 7 files changed, 162 insertions(+), 143 deletions(-) diff --git a/cmd/serv.go b/cmd/serv.go index d7fe6c6630..0326656f2a 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -259,12 +259,16 @@ func runServ(c *cli.Context) error { url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, username, repo.Name) now := time.Now() - token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ + claims := jwt.MapClaims{ "repo": repo.ID, "op": lfsVerb, "exp": now.Add(5 * time.Minute).Unix(), "nbf": now.Unix(), - }) + } + if user != nil { + claims["user"] = user.ID + } + token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) // Sign and get the complete encoded token as a string using the secret tokenString, err := token.SignedString(setting.LFS.JWTSecretBytes) diff --git a/integrations/api_repo_lfs_locks_test.go b/integrations/api_repo_lfs_locks_test.go index 61e554634c..35b26e45ab 100644 --- a/integrations/api_repo_lfs_locks_test.go +++ b/integrations/api_repo_lfs_locks_test.go @@ -41,10 +41,10 @@ func TestAPILFSLocksNotLogin(t *testing.T) { req := NewRequestf(t, "GET", "/%s/%s.git/info/lfs/locks", user.Name, repo.Name) req.Header.Set("Accept", "application/vnd.git-lfs+json") - resp := MakeRequest(t, req, http.StatusForbidden) + resp := MakeRequest(t, req, http.StatusUnauthorized) var lfsLockError api.LFSLockError DecodeJSON(t, resp, &lfsLockError) - assert.Equal(t, "You must have pull access to list locks : User undefined doesn't have rigth to list for lfs lock [rid: 1]", lfsLockError.Message) + assert.Equal(t, "Unauthorized", lfsLockError.Message) } func TestAPILFSLocksLogged(t *testing.T) { @@ -68,8 +68,8 @@ func TestAPILFSLocksLogged(t *testing.T) { {user: user2, repo: repo1, path: "path/test", httpResult: http.StatusConflict}, {user: user2, repo: repo1, path: "Foo/BaR.zip", httpResult: http.StatusConflict}, {user: user2, repo: repo1, path: "Foo/Test/../subFOlder/../Relative/../BaR.zip", httpResult: http.StatusConflict}, - {user: user4, repo: repo1, path: "FoO/BaR.zip", httpResult: http.StatusForbidden}, - {user: user4, repo: repo1, path: "path/test-user4", httpResult: http.StatusForbidden}, + {user: user4, repo: repo1, path: "FoO/BaR.zip", httpResult: http.StatusUnauthorized}, + {user: user4, repo: repo1, path: "path/test-user4", httpResult: http.StatusUnauthorized}, {user: user2, repo: repo1, path: "patH/Test-user4", httpResult: http.StatusCreated, addTime: []int{0}}, {user: user2, repo: repo1, path: "some/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/path", httpResult: http.StatusCreated, addTime: []int{0}}, diff --git a/integrations/git_test.go b/integrations/git_test.go index 05b2c366b1..1ab558d7b7 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -214,11 +214,9 @@ func TestGit(t *testing.T) { commitAndPush(t, bigSize, dstPath) }) }) - /* Failed without #3152. TODO activate with fix. t.Run("Locks", func(t *testing.T) { - lockTest(t, u.String(), dstPath) + lockTest(t, u.String(), dstPath) }) - */ }) }) }) diff --git a/models/error.go b/models/error.go index 765b8fa6ca..cd96fa9256 100644 --- a/models/error.go +++ b/models/error.go @@ -530,21 +530,24 @@ func (err ErrLFSLockNotExist) Error() string { return fmt.Sprintf("lfs lock does not exist [id: %d, rid: %d, path: %s]", err.ID, err.RepoID, err.Path) } -// ErrLFSLockUnauthorizedAction represents a "LFSLockUnauthorizedAction" kind of error. -type ErrLFSLockUnauthorizedAction struct { +// ErrLFSUnauthorizedAction represents a "LFSUnauthorizedAction" kind of error. +type ErrLFSUnauthorizedAction struct { RepoID int64 UserName string - Action string + Mode AccessMode } -// IsErrLFSLockUnauthorizedAction checks if an error is a ErrLFSLockUnauthorizedAction. -func IsErrLFSLockUnauthorizedAction(err error) bool { - _, ok := err.(ErrLFSLockUnauthorizedAction) +// IsErrLFSUnauthorizedAction checks if an error is a ErrLFSUnauthorizedAction. +func IsErrLFSUnauthorizedAction(err error) bool { + _, ok := err.(ErrLFSUnauthorizedAction) return ok } -func (err ErrLFSLockUnauthorizedAction) Error() string { - return fmt.Sprintf("User %s doesn't have rigth to %s for lfs lock [rid: %d]", err.UserName, err.Action, err.RepoID) +func (err ErrLFSUnauthorizedAction) Error() string { + if err.Mode == AccessModeWrite { + return fmt.Sprintf("User %s doesn't have write access for lfs lock [rid: %d]", err.UserName, err.RepoID) + } + return fmt.Sprintf("User %s doesn't have read access for lfs lock [rid: %d]", err.UserName, err.RepoID) } // ErrLFSLockAlreadyExist represents a "LFSLockAlreadyExist" kind of error. diff --git a/models/lfs_lock.go b/models/lfs_lock.go index 9bb87843a3..52e877b156 100644 --- a/models/lfs_lock.go +++ b/models/lfs_lock.go @@ -11,28 +11,40 @@ import ( "strings" "time" + "code.gitea.io/gitea/modules/log" api "code.gitea.io/sdk/gitea" + "github.com/go-xorm/xorm" ) // LFSLock represents a git lfs lock of repository. type LFSLock struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"INDEX NOT NULL"` - Owner *User `xorm:"-"` - OwnerID int64 `xorm:"INDEX NOT NULL"` - Path string `xorm:"TEXT"` - Created time.Time `xorm:"created"` + ID int64 `xorm:"pk autoincr"` + Repo *Repository `xorm:"-"` + RepoID int64 `xorm:"INDEX NOT NULL"` + Owner *User `xorm:"-"` + OwnerID int64 `xorm:"INDEX NOT NULL"` + Path string `xorm:"TEXT"` + Created time.Time `xorm:"created"` } // BeforeInsert is invoked from XORM before inserting an object of this type. func (l *LFSLock) BeforeInsert() { l.OwnerID = l.Owner.ID + l.RepoID = l.Repo.ID l.Path = cleanPath(l.Path) } // AfterLoad is invoked from XORM after setting the values of all fields of this object. -func (l *LFSLock) AfterLoad() { - l.Owner, _ = GetUserByID(l.OwnerID) +func (l *LFSLock) AfterLoad(session *xorm.Session) { + var err error + l.Owner, err = getUserByID(session, l.OwnerID) + if err != nil { + log.Error(2, "LFS lock AfterLoad failed OwnerId[%d] not found: %v", l.OwnerID, err) + } + l.Repo, err = getRepositoryByID(session, l.RepoID) + if err != nil { + log.Error(2, "LFS lock AfterLoad failed RepoId[%d] not found: %v", l.RepoID, err) + } } func cleanPath(p string) string { @@ -53,12 +65,12 @@ func (l *LFSLock) APIFormat() *api.LFSLock { // CreateLFSLock creates a new lock. func CreateLFSLock(lock *LFSLock) (*LFSLock, error) { - err := CheckLFSAccessForRepo(lock.Owner, lock.RepoID, "create") + err := CheckLFSAccessForRepo(lock.Owner, lock.Repo, AccessModeWrite) if err != nil { return nil, err } - l, err := GetLFSLock(lock.RepoID, lock.Path) + l, err := GetLFSLock(lock.Repo, lock.Path) if err == nil { return l, ErrLFSLockAlreadyExist{lock.RepoID, lock.Path} } @@ -71,15 +83,15 @@ func CreateLFSLock(lock *LFSLock) (*LFSLock, error) { } // GetLFSLock returns release by given path. -func GetLFSLock(repoID int64, path string) (*LFSLock, error) { +func GetLFSLock(repo *Repository, path string) (*LFSLock, error) { path = cleanPath(path) - rel := &LFSLock{RepoID: repoID} + rel := &LFSLock{RepoID: repo.ID} has, err := x.Where("lower(path) = ?", strings.ToLower(path)).Get(rel) if err != nil { return nil, err } if !has { - return nil, ErrLFSLockNotExist{0, repoID, path} + return nil, ErrLFSLockNotExist{0, repo.ID, path} } return rel, nil } @@ -109,7 +121,7 @@ func DeleteLFSLockByID(id int64, u *User, force bool) (*LFSLock, error) { return nil, err } - err = CheckLFSAccessForRepo(u, lock.RepoID, "delete") + err = CheckLFSAccessForRepo(u, lock.Repo, AccessModeWrite) if err != nil { return nil, err } @@ -123,24 +135,15 @@ func DeleteLFSLockByID(id int64, u *User, force bool) (*LFSLock, error) { } //CheckLFSAccessForRepo check needed access mode base on action -func CheckLFSAccessForRepo(u *User, repoID int64, action string) error { +func CheckLFSAccessForRepo(u *User, repo *Repository, mode AccessMode) error { if u == nil { - return ErrLFSLockUnauthorizedAction{repoID, "undefined", action} - } - mode := AccessModeRead - if action == "create" || action == "delete" || action == "verify" { - mode = AccessModeWrite - } - - repo, err := GetRepositoryByID(repoID) - if err != nil { - return err + return ErrLFSUnauthorizedAction{repo.ID, "undefined", mode} } has, err := HasAccess(u.ID, repo, mode) if err != nil { return err } else if !has { - return ErrLFSLockUnauthorizedAction{repo.ID, u.DisplayName(), action} + return ErrLFSUnauthorizedAction{repo.ID, u.DisplayName(), mode} } return nil } diff --git a/modules/lfs/locks.go b/modules/lfs/locks.go index 2e776c26a0..7a7d3ad42a 100644 --- a/modules/lfs/locks.go +++ b/modules/lfs/locks.go @@ -13,24 +13,35 @@ import ( "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/sdk/gitea" - - "gopkg.in/macaron.v1" ) -func checkRequest(req macaron.Request, post bool) int { +//checkIsValidRequest check if it a valid request in case of bad request it write the response to ctx. +func checkIsValidRequest(ctx *context.Context, post bool) bool { if !setting.LFS.StartServer { - return 404 + writeStatus(ctx, 404) + return false } - if !MetaMatcher(req) { - return 400 + if !MetaMatcher(ctx.Req) { + writeStatus(ctx, 400) + return false + } + if !ctx.IsSigned { + user, _, _, err := parseToken(ctx.Req.Header.Get("Authorization")) + if err != nil { + ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs") + writeStatus(ctx, 401) + return false + } + ctx.User = user } if post { - mediaParts := strings.Split(req.Header.Get("Content-Type"), ";") + mediaParts := strings.Split(ctx.Req.Header.Get("Content-Type"), ";") if mediaParts[0] != metaMediaType { - return 400 + writeStatus(ctx, 400) + return false } } - return 200 + return true } func handleLockListOut(ctx *context.Context, lock *models.LFSLock, err error) { @@ -59,17 +70,16 @@ func handleLockListOut(ctx *context.Context, lock *models.LFSLock, err error) { // GetListLockHandler list locks func GetListLockHandler(ctx *context.Context) { - status := checkRequest(ctx.Req, false) - if status != 200 { - writeStatus(ctx, status) + if !checkIsValidRequest(ctx, false) { return } ctx.Resp.Header().Set("Content-Type", metaMediaType) - err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository.ID, "list") + err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository, models.AccessModeRead) if err != nil { - if models.IsErrLFSLockUnauthorizedAction(err) { - ctx.JSON(403, api.LFSLockError{ + if models.IsErrLFSUnauthorizedAction(err) { + ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs") + ctx.JSON(401, api.LFSLockError{ Message: "You must have pull access to list locks : " + err.Error(), }) return @@ -96,7 +106,7 @@ func GetListLockHandler(ctx *context.Context) { path := ctx.Query("path") if path != "" { //Case where we request a specific id - lock, err := models.GetLFSLock(ctx.Repo.Repository.ID, path) + lock, err := models.GetLFSLock(ctx.Repo.Repository, path) handleLockListOut(ctx, lock, err) return } @@ -120,9 +130,7 @@ func GetListLockHandler(ctx *context.Context) { // PostLockHandler create lock func PostLockHandler(ctx *context.Context) { - status := checkRequest(ctx.Req, true) - if status != 200 { - writeStatus(ctx, status) + if !checkIsValidRequest(ctx, false) { return } ctx.Resp.Header().Set("Content-Type", metaMediaType) @@ -136,9 +144,9 @@ func PostLockHandler(ctx *context.Context) { } lock, err := models.CreateLFSLock(&models.LFSLock{ - RepoID: ctx.Repo.Repository.ID, - Path: req.Path, - Owner: ctx.User, + Repo: ctx.Repo.Repository, + Path: req.Path, + Owner: ctx.User, }) if err != nil { if models.IsErrLFSLockAlreadyExist(err) { @@ -148,8 +156,9 @@ func PostLockHandler(ctx *context.Context) { }) return } - if models.IsErrLFSLockUnauthorizedAction(err) { - ctx.JSON(403, api.LFSLockError{ + if models.IsErrLFSUnauthorizedAction(err) { + ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs") + ctx.JSON(401, api.LFSLockError{ Message: "You must have push access to create locks : " + err.Error(), }) return @@ -164,18 +173,16 @@ func PostLockHandler(ctx *context.Context) { // VerifyLockHandler list locks for verification func VerifyLockHandler(ctx *context.Context) { - status := checkRequest(ctx.Req, true) - if status != 200 { - writeStatus(ctx, status) + if !checkIsValidRequest(ctx, false) { return } - ctx.Resp.Header().Set("Content-Type", metaMediaType) - err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository.ID, "verify") + err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository, models.AccessModeWrite) if err != nil { - if models.IsErrLFSLockUnauthorizedAction(err) { - ctx.JSON(403, api.LFSLockError{ + if models.IsErrLFSUnauthorizedAction(err) { + ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs") + ctx.JSON(401, api.LFSLockError{ Message: "You must have push access to verify locks : " + err.Error(), }) return @@ -211,9 +218,7 @@ func VerifyLockHandler(ctx *context.Context) { // UnLockHandler delete locks func UnLockHandler(ctx *context.Context) { - status := checkRequest(ctx.Req, true) - if status != 200 { - writeStatus(ctx, status) + if !checkIsValidRequest(ctx, false) { return } ctx.Resp.Header().Set("Content-Type", metaMediaType) @@ -228,8 +233,9 @@ func UnLockHandler(ctx *context.Context) { lock, err := models.DeleteLFSLockByID(ctx.ParamsInt64("lid"), ctx.User, req.Force) if err != nil { - if models.IsErrLFSLockUnauthorizedAction(err) { - ctx.JSON(403, api.LFSLockError{ + if models.IsErrLFSUnauthorizedAction(err) { + ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs") + ctx.JSON(401, api.LFSLockError{ Message: "You must have push access to delete locks : " + err.Error(), }) return diff --git a/modules/lfs/server.go b/modules/lfs/server.go index 329d6f00c4..a81d8e5c91 100644 --- a/modules/lfs/server.go +++ b/modules/lfs/server.go @@ -473,7 +473,6 @@ func logRequest(r macaron.Request, status int) { // authenticate uses the authorization string to determine whether // or not to proceed. This server assumes an HTTP Basic auth format. func authenticate(ctx *context.Context, repository *models.Repository, authorization string, requireWrite bool) bool { - accessMode := models.AccessModeRead if requireWrite { accessMode = models.AccessModeWrite @@ -482,86 +481,92 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza if !repository.IsPrivate && !requireWrite { return true } - if ctx.IsSigned { accessCheck, _ := models.HasAccess(ctx.User.ID, repository, accessMode) return accessCheck } - if authorization == "" { + user, repo, opStr, err := parseToken(authorization) + if err != nil { return false } - - if authenticateToken(repository, authorization, requireWrite) { + ctx.User = user + if opStr == "basic" { + accessCheck, _ := models.HasAccess(ctx.User.ID, repository, accessMode) + return accessCheck + } + if repository.ID == repo.ID { + if requireWrite && opStr != "upload" { + return false + } return true } - - if !strings.HasPrefix(authorization, "Basic ") { - return false - } - - c, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(authorization, "Basic ")) - if err != nil { - return false - } - cs := string(c) - i := strings.IndexByte(cs, ':') - if i < 0 { - return false - } - user, password := cs[:i], cs[i+1:] - - userModel, err := models.GetUserByName(user) - if err != nil { - return false - } - - if !userModel.ValidatePassword(password) { - return false - } - - accessCheck, _ := models.HasAccess(userModel.ID, repository, accessMode) - return accessCheck + return false } -func authenticateToken(repository *models.Repository, authorization string, requireWrite bool) bool { - if !strings.HasPrefix(authorization, "Bearer ") { - return false +func parseToken(authorization string) (*models.User, *models.Repository, string, error) { + if authorization == "" { + return nil, nil, "unknown", fmt.Errorf("No token") } - - token, err := jwt.Parse(authorization[7:], func(t *jwt.Token) (interface{}, error) { - if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { - return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) + if strings.HasPrefix(authorization, "Bearer ") { + token, err := jwt.Parse(authorization[7:], func(t *jwt.Token) (interface{}, error) { + if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { + return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) + } + return setting.LFS.JWTSecretBytes, nil + }) + if err != nil { + return nil, nil, "unknown", err } - return setting.LFS.JWTSecretBytes, nil - }) - if err != nil { - return false - } - claims, claimsOk := token.Claims.(jwt.MapClaims) - if !token.Valid || !claimsOk { - return false + claims, claimsOk := token.Claims.(jwt.MapClaims) + if !token.Valid || !claimsOk { + return nil, nil, "unknown", fmt.Errorf("Token claim invalid") + } + opStr, ok := claims["op"].(string) + if !ok { + return nil, nil, "unknown", fmt.Errorf("Token operation invalid") + } + repoID, ok := claims["repo"].(float64) + if !ok { + return nil, nil, opStr, fmt.Errorf("Token repository id invalid") + } + r, err := models.GetRepositoryByID(int64(repoID)) + if err != nil { + return nil, nil, opStr, err + } + userID, ok := claims["user"].(float64) + if !ok { + return nil, r, opStr, fmt.Errorf("Token user id invalid") + } + u, err := models.GetUserByID(int64(userID)) + if err != nil { + return nil, r, opStr, err + } + return u, r, opStr, nil } - opStr, ok := claims["op"].(string) - if !ok { - return false + if strings.HasPrefix(authorization, "Basic ") { + c, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(authorization, "Basic ")) + if err != nil { + return nil, nil, "basic", err + } + cs := string(c) + i := strings.IndexByte(cs, ':') + if i < 0 { + return nil, nil, "basic", fmt.Errorf("Basic auth invalid") + } + user, password := cs[:i], cs[i+1:] + u, err := models.GetUserByName(user) + if err != nil { + return nil, nil, "basic", err + } + if !u.ValidatePassword(password) { + return nil, nil, "basic", fmt.Errorf("Basic auth failed") + } + return u, nil, "basic", nil } - if requireWrite && opStr != "upload" { - return false - } - - repoID, ok := claims["repo"].(float64) - if !ok { - return false - } - - if repository.ID != int64(repoID) { - return false - } - - return true + return nil, nil, "unknown", fmt.Errorf("Token not found") } func requireAuth(ctx *context.Context) {