From 14f919df47c18868fe878fbf49b57d5581daa3d6 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 21 Jul 2019 00:19:17 +0200 Subject: [PATCH] simplify some code --- api/api_utils.go | 14 ++- go.mod | 1 + go.sum | 2 + sftpd/handler.go | 269 ++++++++++++++++++++++++++------------------ sftpd/sftpd_test.go | 85 +++++++++++--- 5 files changed, 240 insertions(+), 131 deletions(-) diff --git a/api/api_utils.go b/api/api_utils.go index 2d734ea6..da288830 100644 --- a/api/api_utils.go +++ b/api/api_utils.go @@ -197,6 +197,15 @@ func checkUser(expected dataprovider.User, actual dataprovider.User) error { return errors.New("user ID mismatch") } } + for _, v := range expected.Permissions { + if !utils.IsStringInSlice(v, actual.Permissions) { + return errors.New("Permissions contents mismatch") + } + } + return compareEqualsUserFields(expected, actual) +} + +func compareEqualsUserFields(expected dataprovider.User, actual dataprovider.User) error { if expected.Username != actual.Username { return errors.New("Username mismatch") } @@ -221,11 +230,6 @@ func checkUser(expected dataprovider.User, actual dataprovider.User) error { if len(expected.Permissions) != len(actual.Permissions) { return errors.New("Permissions mismatch") } - for _, v := range expected.Permissions { - if !utils.IsStringInSlice(v, actual.Permissions) { - return errors.New("Permissions contents mismatch") - } - } if expected.UploadBandwidth != actual.UploadBandwidth { return errors.New("UploadBandwidth mismatch") } diff --git a/go.mod b/go.mod index e81f4b19..e64604d1 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.12 require ( github.com/alexedwards/argon2id v0.0.0-20190612080829-01a59b2b8802 + github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835 // indirect github.com/go-chi/chi v4.0.2+incompatible github.com/go-chi/render v1.0.1 github.com/go-sql-driver/mysql v1.4.1 diff --git a/go.sum b/go.sum index 94c2d0c2..05c1b25d 100644 --- a/go.sum +++ b/go.sum @@ -3,6 +3,8 @@ github.com/alexedwards/argon2id v0.0.0-20190612080829-01a59b2b8802/go.mod h1:4ds github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835 h1:roDmqJ4Qes7hrDOsWsMCce0vQHz3xiMPjJ9m4c2eeNs= +github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835/go.mod h1:BjL/N0+C+j9uNX+1xcNuM9vdSIcXCZrQZUYbXOFbgN8= github.com/go-chi/chi v4.0.2+incompatible h1:maB6vn6FqCxrpz4FqWdh4+lwpyZIQS7YEAUcHlgXVRs= github.com/go-chi/chi v4.0.2+incompatible/go.mod h1:eB3wogJHnLi3x/kFX2A+IbTBlXxmMeXJVKy9tTv1XzQ= github.com/go-chi/render v1.0.1 h1:4/5tis2cKaNdnv9zFLfXzcquC9HbeZgCnxGnKrltBS8= diff --git a/sftpd/handler.go b/sftpd/handler.go index 3b60326a..a78d2c14 100644 --- a/sftpd/handler.go +++ b/sftpd/handler.go @@ -106,21 +106,12 @@ func (c Connection) Filewrite(request *sftp.Request) (io.WriterAt, error) { } } - dirsToCreate, err := c.findNonexistentDirs(p) + err = c.createMissingDirs(p) if err != nil { + logger.Error(logSender, "error making missing dir for path %v: %v", p, err) return nil, sftp.ErrSshFxFailure } - last := len(dirsToCreate) - 1 - for i := range dirsToCreate { - d := dirsToCreate[last-i] - if err := os.Mkdir(d, 0777); err != nil { - logger.Error(logSender, "error making path for file, dir: %v, path: %v", d, p) - return nil, sftp.ErrSshFxFailure - } - utils.SetPathPermissions(d, c.User.GetUID(), c.User.GetGID()) - } - file, err := os.Create(p) if err != nil { logger.Error(logSender, "error creating file %v: %v", p, err) @@ -162,24 +153,7 @@ func (c Connection) Filewrite(request *sftp.Request) (io.WriterAt, error) { return nil, sftp.ErrSshFxOpUnsupported } - var osFlags int - trunc := false - sftpFileOpenFlags := request.Pflags() - if sftpFileOpenFlags.Read && sftpFileOpenFlags.Write { - osFlags |= os.O_RDWR - } else if sftpFileOpenFlags.Write { - osFlags |= os.O_WRONLY - } - if sftpFileOpenFlags.Append { - osFlags |= os.O_APPEND - } - if sftpFileOpenFlags.Creat { - osFlags |= os.O_CREATE - } - if sftpFileOpenFlags.Trunc { - osFlags |= os.O_TRUNC - trunc = true - } + osFlags, trunc := getOSOpenFlags(request.Pflags()) if !trunc { // see https://github.com/pkg/sftp/issues/295 @@ -228,109 +202,44 @@ func (c Connection) Filecmd(request *sftp.Request) error { return sftp.ErrSshFxNoSuchFile } - var target string - // If a target is provided in this request validate that it is going to the correct - // location for the server. If it is not, return an operation unsupported error. This - // is maybe not the best error response, but its not wrong either. - if request.Target != "" { - target, err = c.buildPath(request.Target) - if err != nil { - return sftp.ErrSshFxOpUnsupported - } + target, err := c.getSFTPCmdTargetPath(request.Target) + if err != nil { + return sftp.ErrSshFxOpUnsupported } - logger.Debug(logSender, "new cmd, method: %v user: %v", request.Method, c.User.Username) + logger.Debug(logSender, "new cmd, method: %v user: %v sourcePath: %v, targetPath: %v", request.Method, c.User.Username, + p, target) switch request.Method { case "Setstat": return nil case "Rename": - if !c.User.HasPerm(dataprovider.PermRename) { - return sftp.ErrSshFxPermissionDenied - } - - logger.CommandLog(sftpdRenameLogSender, p, target, c.User.Username, c.ID) - if err := os.Rename(p, target); err != nil { - logger.Error("failed to rename file, source: %v target: %v: %v", p, target, err) - return sftp.ErrSshFxFailure + err = c.handleSFTPRename(p, target) + if err != nil { + return err } break case "Rmdir": - if !c.User.HasPerm(dataprovider.PermDelete) { - return sftp.ErrSshFxPermissionDenied - } + return c.handleSFTPRmdir(p) - logger.CommandLog(sftpdRmdirLogSender, p, target, c.User.Username, c.ID) - numFiles, size, err := utils.ScanDirContents(p) - if err != nil { - logger.Error("failed to remove directory %v, scanning error: %v", p, err) - return sftp.ErrSshFxFailure - } - if err := os.RemoveAll(p); err != nil { - logger.Error("failed to remove directory %v: %v", p, err) - return sftp.ErrSshFxFailure - } - - dataprovider.UpdateUserQuota(dataProvider, c.User.Username, -numFiles, -size, false) - - return sftp.ErrSshFxOk case "Mkdir": - if !c.User.HasPerm(dataprovider.PermCreateDirs) { - return sftp.ErrSshFxPermissionDenied - } - - logger.CommandLog(sftpdMkdirLogSender, p, target, c.User.Username, c.ID) - dirsToCreate, err := c.findNonexistentDirs(filepath.Join(p, "testfile")) + err = c.handleSFTPMkdir(p) if err != nil { - return sftp.ErrSshFxFailure + return err } - last := len(dirsToCreate) - 1 - for i := range dirsToCreate { - d := dirsToCreate[last-i] - if err := os.Mkdir(d, 0777); err != nil { - logger.Error(logSender, "error making path dir: %v, full path: %v", d, p) - return sftp.ErrSshFxFailure - } - utils.SetPathPermissions(d, c.User.GetUID(), c.User.GetGID()) - } break case "Symlink": - if !c.User.HasPerm(dataprovider.PermCreateSymlinks) { - return sftp.ErrSshFxPermissionDenied - } - - logger.CommandLog(sftpdSymlinkLogSender, p, target, c.User.Username, c.ID) - if err := os.Symlink(p, target); err != nil { - logger.Warn("failed to create symlink %v->%v: %v", p, target, err) - return sftp.ErrSshFxFailure + err = c.handleSFTPSymlink(p, target) + if err != nil { + return err } break case "Remove": - if !c.User.HasPerm(dataprovider.PermDelete) { - return sftp.ErrSshFxPermissionDenied - } + return c.handleSFTPRemove(p) - logger.CommandLog(sftpdRemoveLogSender, p, target, c.User.Username, c.ID) - var size int64 - var fi os.FileInfo - if fi, err = os.Lstat(p); err != nil { - logger.Error(logSender, "failed to remove a file %v: stat error: %v", p, err) - return sftp.ErrSshFxFailure - } - size = fi.Size() - if err := os.Remove(p); err != nil { - logger.Error(logSender, "failed to remove a file %v: %v", p, err) - return sftp.ErrSshFxFailure - } - - if fi.Mode()&os.ModeSymlink != os.ModeSymlink { - dataprovider.UpdateUserQuota(dataProvider, c.User.Username, -1, -size, false) - } - - return sftp.ErrSshFxOk default: return sftp.ErrSshFxOpUnsupported } @@ -340,6 +249,7 @@ func (c Connection) Filecmd(request *sftp.Request) error { fileLocation = target } + // we return if we remove a file or a dir so source path or target path always exists here utils.SetPathPermissions(fileLocation, c.User.GetUID(), c.User.GetGID()) return sftp.ErrSshFxOk @@ -389,6 +299,106 @@ func (c Connection) Filelist(request *sftp.Request) (sftp.ListerAt, error) { } } +func (c Connection) getSFTPCmdTargetPath(requestTarget string) (string, error) { + var target string + // If a target is provided in this request validate that it is going to the correct + // location for the server. If it is not, return an operation unsupported error. This + // is maybe not the best error response, but its not wrong either. + if requestTarget != "" { + var err error + target, err = c.buildPath(requestTarget) + if err != nil { + return target, sftp.ErrSshFxOpUnsupported + } + } + return target, nil +} + +func (c Connection) handleSFTPRename(sourcePath string, targetPath string) error { + if !c.User.HasPerm(dataprovider.PermRename) { + return sftp.ErrSshFxPermissionDenied + } + if err := os.Rename(sourcePath, targetPath); err != nil { + logger.Error(logSender, "failed to rename file, source: %v target: %v: %v", sourcePath, targetPath, err) + return sftp.ErrSshFxFailure + } + logger.CommandLog(sftpdRenameLogSender, sourcePath, targetPath, c.User.Username, c.ID) + return nil +} + +func (c Connection) handleSFTPRmdir(path string) error { + if !c.User.HasPerm(dataprovider.PermDelete) { + return sftp.ErrSshFxPermissionDenied + } + + numFiles, size, err := utils.ScanDirContents(path) + if err != nil { + logger.Error(logSender, "failed to remove directory %v, scanning error: %v", path, err) + return sftp.ErrSshFxFailure + } + if err := os.RemoveAll(path); err != nil { + logger.Error(logSender, "failed to remove directory %v: %v", path, err) + return sftp.ErrSshFxFailure + } + + logger.CommandLog(sftpdRmdirLogSender, path, "", c.User.Username, c.ID) + dataprovider.UpdateUserQuota(dataProvider, c.User.Username, -numFiles, -size, false) + + return sftp.ErrSshFxOk +} + +func (c Connection) handleSFTPSymlink(sourcePath string, targetPath string) error { + if !c.User.HasPerm(dataprovider.PermCreateSymlinks) { + return sftp.ErrSshFxPermissionDenied + } + if err := os.Symlink(sourcePath, targetPath); err != nil { + logger.Warn(logSender, "failed to create symlink %v -> %v: %v", sourcePath, targetPath, err) + return sftp.ErrSshFxFailure + } + + logger.CommandLog(sftpdSymlinkLogSender, sourcePath, targetPath, c.User.Username, c.ID) + return nil +} + +func (c Connection) handleSFTPMkdir(path string) error { + if !c.User.HasPerm(dataprovider.PermCreateDirs) { + return sftp.ErrSshFxPermissionDenied + } + + if err := c.createMissingDirs(filepath.Join(path, "testfile")); err != nil { + logger.Error(logSender, "error making missing dir for path %v: %v", path, err) + return sftp.ErrSshFxFailure + } + logger.CommandLog(sftpdMkdirLogSender, path, "", c.User.Username, c.ID) + return nil +} + +func (c Connection) handleSFTPRemove(path string) error { + if !c.User.HasPerm(dataprovider.PermDelete) { + return sftp.ErrSshFxPermissionDenied + } + + var size int64 + var fi os.FileInfo + var err error + if fi, err = os.Lstat(path); err != nil { + logger.Error(logSender, "failed to remove a file %v: stat error: %v", path, err) + return sftp.ErrSshFxFailure + } + size = fi.Size() + if err := os.Remove(path); err != nil { + logger.Error(logSender, "failed to remove a file/symlink %v: %v", path, err) + return sftp.ErrSshFxFailure + } + + logger.CommandLog(sftpdRemoveLogSender, path, "", c.User.Username, c.ID) + if fi.Mode()&os.ModeSymlink != os.ModeSymlink { + dataprovider.UpdateUserQuota(dataProvider, c.User.Username, -1, -size, false) + } + + return sftp.ErrSshFxOk +} + func (c Connection) hasSpace(checkFiles bool) bool { if (checkFiles && c.User.QuotaFiles > 0) || c.User.QuotaSize > 0 { numFile, size, err := dataprovider.GetUsedQuota(c.dataProvider, c.User.Username) @@ -505,3 +515,44 @@ func (c Connection) isSubDir(sub string) error { } return nil } + +func (c Connection) createMissingDirs(filePath string) error { + dirsToCreate, err := c.findNonexistentDirs(filePath) + if err != nil { + return err + } + last := len(dirsToCreate) - 1 + for i := range dirsToCreate { + d := dirsToCreate[last-i] + if err := os.Mkdir(d, 0777); err != nil { + logger.Error(logSender, "error creating missing dir: %v", d) + return err + } + utils.SetPathPermissions(d, c.User.GetUID(), c.User.GetGID()) + } + return nil +} + +func getOSOpenFlags(requestFlags sftp.FileOpenFlags) (flags int, trunc bool) { + var osFlags int + truncateFile := false + if requestFlags.Read && requestFlags.Write { + osFlags |= os.O_RDWR + } else if requestFlags.Write { + osFlags |= os.O_WRONLY + } + if requestFlags.Append { + osFlags |= os.O_APPEND + } + if requestFlags.Creat { + osFlags |= os.O_CREATE + } + if requestFlags.Trunc { + osFlags |= os.O_TRUNC + truncateFile = true + } + if requestFlags.Excl { + osFlags |= os.O_EXCL + } + return osFlags, truncateFile +} diff --git a/sftpd/sftpd_test.go b/sftpd/sftpd_test.go index f37d13be..45269f19 100644 --- a/sftpd/sftpd_test.go +++ b/sftpd/sftpd_test.go @@ -23,7 +23,7 @@ import ( const ( sftpServerAddr = "127.0.0.1:2022" - defaultUsername = "test_user" + defaultUsername = "test_user_sftp" defaultPassword = "test_password" testPubKey = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC03jj0D+djk7pxIf/0OhrxrchJTRZklofJ1NoIu4752Sq02mdXmarMVsqJ1cAjV5LBVy3D1F5U6XW4rppkXeVtd04Pxb09ehtH0pRRPaoHHlALiJt8CoMpbKYMA8b3KXPPriGxgGomvtU2T2RMURSwOZbMtpsugfjYSWenyYX+VORYhylWnSXL961LTyC21ehd6d6QnW9G7E5hYMITMY9TuQZz3bROYzXiTsgN0+g6Hn7exFQp50p45StUMfV/SftCMdCxlxuyGny2CrN/vfjO7xxOo2uv7q1qm10Q46KPWJQv+pgZ/OfL+EDjy07n5QVSKHlbx+2nT4Q0EgOSQaCTYwn3YjtABfIxWwgAFdyj6YlPulCL22qU4MYhDcA6PSBwDdf8hvxBfvsiHdM+JcSHvv8/VeJhk6CmnZxGY0fxBupov27z3yEO8nAg8k+6PaUiW1MSUfuGMF/ktB8LOstXsEPXSszuyXiOv4DaryOXUiSn7bmRqKcEFlJusO6aZP0= nicola@p1" testPrivateKey = `-----BEGIN OPENSSH PRIVATE KEY----- @@ -193,14 +193,6 @@ func TestBasicSFTPHandling(t *testing.T) { t.Errorf("unable to create sftp client: %v", err) } else { defer client.Close() - _, err := client.Getwd() - if err != nil { - t.Errorf("unable to get working dir: %v", err) - } - _, err = client.ReadDir(".") - if err != nil { - t.Errorf("unable to read remote dir: %v", err) - } testFileName := "test_file.dat" testFilePath := filepath.Join(homeBasePath, testFileName) testFileSize := int64(65535) @@ -214,14 +206,6 @@ func TestBasicSFTPHandling(t *testing.T) { if err != nil { t.Errorf("file upload error: %v", err) } - err = client.Symlink(testFileName, testFileName+".link") - if err != nil { - t.Errorf("error creating symlink: %v", err) - } - err = client.Remove(testFileName + ".link") - if err != nil { - t.Errorf("error removing symlink: %v", err) - } localDownloadPath := filepath.Join(homeBasePath, "test_download.dat") err = sftpDownloadFile(testFileName, localDownloadPath, testFileSize, client) if err != nil { @@ -255,6 +239,32 @@ func TestBasicSFTPHandling(t *testing.T) { if (expectedQuotaSize - testFileSize) != user.UsedQuotaSize { t.Errorf("quota size does not match, expected: %v, actual: %v", expectedQuotaSize-testFileSize, user.UsedQuotaSize) } + } + err = api.RemoveUser(user, http.StatusOK) + if err != nil { + t.Errorf("unable to remove user: %v", err) + } +} + +func TestDirCommands(t *testing.T) { + usePubKey := false + user, err := api.AddUser(getTestUser(usePubKey), http.StatusOK) + if err != nil { + t.Errorf("unable to add user: %v", err) + } + client, err := getSftpClient(user, usePubKey) + if err != nil { + t.Errorf("unable to create sftp client: %v", err) + } else { + defer client.Close() + _, err := client.Getwd() + if err != nil { + t.Errorf("unable to get working dir: %v", err) + } + _, err = client.ReadDir(".") + if err != nil { + t.Errorf("unable to read remote dir: %v", err) + } err = client.Mkdir("test") if err != nil { t.Errorf("error mkdir: %v", err) @@ -286,6 +296,47 @@ func TestBasicSFTPHandling(t *testing.T) { } } +func TestSymlink(t *testing.T) { + usePubKey := false + user, err := api.AddUser(getTestUser(usePubKey), http.StatusOK) + if err != nil { + t.Errorf("unable to add user: %v", err) + } + client, err := getSftpClient(user, usePubKey) + if err != nil { + t.Errorf("unable to create sftp client: %v", err) + } else { + defer client.Close() + testFileName := "test_file.dat" + testFilePath := filepath.Join(homeBasePath, testFileName) + testFileSize := int64(65535) + err = createTestFile(testFilePath, testFileSize) + if err != nil { + t.Errorf("unable to create test file: %v", err) + } + err = sftpUploadFile(testFilePath, testFileName, testFileSize, client) + if err != nil { + t.Errorf("file upload error: %v", err) + } + err = client.Symlink(testFileName, testFileName+".link") + if err != nil { + t.Errorf("error creating symlink: %v", err) + } + err = client.Remove(testFileName + ".link") + if err != nil { + t.Errorf("error removing symlink: %v", err) + } + err = client.Remove(testFileName) + if err != nil { + t.Errorf("error removing uploaded file: %v", err) + } + } + err = api.RemoveUser(user, http.StatusOK) + if err != nil { + t.Errorf("unable to remove user: %v", err) + } +} + // basic tests to verify virtual chroot, should be improved to cover more cases ... func TestEscapeHomeDir(t *testing.T) { usePubKey := true