SFTP support for the go.crypto/ssh package

I'm using this code. It mostly works, except when copying a large file with WinSCP I get the error "Incoming packet was garbled on decryption" and I have to keep reconnecting.

According to the WinSCP docs this could be because of miscomputing SSH-2 encryption keys, or ignoring max packet lengths. I tried enabling both workarounds in WinSCP but to no avail.

Entirely possible my code is doing something wrong (I really have very little idea how SSH works; it's mostly copy-pasta). Buuut it could also be a bug in this code, so .... any ideas?

While investigating https://github.com/rclone/rclone/issues/5197 I discovered that doing transfers to rsync.net had gone from 300 KB/s in v1.12.0 to 100KB/s in v1.13.0

The rsync.net servers run FreeBSD which is one unusual thing about them and the other is that they are a long way away from me (150ms) so have the usual problems with long fat TCP pipes. Rclone uses the ReadFrom interface in the sftp client so that the sftp library can increase the number of outstanding packets to help with this.

Here are some tests (my upload is capable of 2MB/s).

make && rclone version && rclone copy -vv --stats 10s /tmp/499.91M rsyncnet: 2>&1 | grep 499.91M:

v1.12.0

 *                                       499.91M:  0% /499.910M, 312.432k/s, 27m8s
 *                                       499.91M:  1% /499.910M, 309.264k/s, 27m15s
 *                                       499.91M:  1% /499.910M, 281.361k/s, 29m49s
 *                                       499.91M:  2% /499.910M, 312.820k/s, 26m37s
 *                                       499.91M:  3% /499.910M, 347.230k/s, 23m48s

v1.13.0

 *                                       499.91M:  0% /499.910M, 81.331k/s, 1h44m44s
 *                                       499.91M:  0% /499.910M, 97.554k/s, 1h27m8s
 *                                       499.91M:  0% /499.910M, 98.655k/s, 1h25m59s
 *                                       499.91M:  0% /499.910M, 122.371k/s, 1h9m7s
 *                                       499.91M:  1% /499.910M, 122.173k/s, 1h9m4s

master

 *                                       499.91M:  0% /499.910M, 109.761k/s, 1h17m33s
 *                                       499.91M:  0% /499.910M, 102.153k/s, 1h23m11s
 *                                       499.91M:  0% /499.910M, 91.254k/s, 1h32m58s
 *                                       499.91M:  0% /499.910M, 85.305k/s, 1h39m18s
 *                                       499.91M:  0% /499.910M, 76.641k/s, 1h50m23s

I bisected this change and discovered this commit is probably the problem. I'm reasonably sure that is the problem commit, and it is certainly touching the code in question. Before this commit at 0be6950c0e91d5cb73a4d690270d3a5010ac9808 the performance is definitely OK and after it is is bad. However there is some variation after the commit so there may be more commits involved.

commit fc156996912168806d33b55794e2f5436fae2c3d Author: Cassondra Foesch Date: Sun Feb 21 20:32:09 2021 +0000

fixed concurrent writes, mid-polish

client.go | 614 ++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 373 insertions(+), 241 deletions(-)

Cc: @puellanivis

• MarshalBinary for packets must now include 4 empty bytes at the start for filling in the length of the packet.
• Packets may now implement marshalPacket that can return both the packet and payload as separate byte-slices, allowing the payload to be written separately without having to copy it into a separate marshaled packet slice first.
• rigorously define behavior of chan result in sending packets to the server to guarantee at-most-once delivery on each channel, meaning we can halve the space overhead from allocating a buffered channel (happens nearly every request).
• fix a write-read race condition in request-server_test.go
• fix a bunch of edge-cases in client_integration_test.go that were causing integration tests to lock up, be flaky or just not work.
• implement WriterAt
• new concurrency model of Map→Worker→Reduce for better high-latency concurrency in: ReadAt, WriteAt, ReadFrom.
• if a ReadAt or WriteAt can be done in one request, short-circuit concurrency and just do the request straight
• try and guess if a ReadFrom can be done in one request, and short-circuit to a synchronous loop (this is to be absolutely sure the io.Reader is read to io.EOF, even though it’s strongly likely that it will only ever run one loop.)
• TODO: currently pondering how best to do WriteTo efficiently with the Map→Worker→Reduce paradigm. For sure though, it won’t end up as similar as the other three are to each other.

Since upgrading to 1.13 the library will often report EOF before a file opened with Client.Open() is downloaded completely. For example a 634618 byte file has stopped downloading after just 65536 bytes, and a 2367599 byte file after 2031616.

As a workaround I've added a call to File.Stat().Size(), which returns the correct file size, and compare that with the downloaded size to check for success.

Hi,

using ws ftp pro (trial latest version) library seems to have issues. This can be replicated using the sftp-server example, having for example 20 files with small amount of data, I tested 22 bytes.

Using command "C:\Program Files (x86)\Ipswitch\WS_FTP 12\wsftppro.exe" -s sftp-lib-conf:/path_to/github.com/pkg/sftp/examples/sftp-server/testfile* -d local:C:\temp\

Some of the files come as empty. Also sometimes just changing the directory in UI fails.

I think this is related to packets coming/going in wrong order. Temporary fix that I found was to set SftpServerWorkerCount=1

I debugged the issue and failing transfers it seems to read with offset greater than filesize, when server responds EOF the next packet with offset zero and correct data seems to be ignored by the client.

In succesfull transfers offset zero packet seems to come first.

This issue is not in openssh server, it seems to always receive offset zero first.

Mikko.

The request server implementation does not handle symlinks correctly. If the link target is relative it is rewritten to an absolute path. The first commit fixes that.

The second commit is just adding a testcase for linking to non-existent files.

We have noticed that Cyberduck versions 4-6 (as far as we've tested) do not properly handle the following case when the library returns the following path:

'/.'

Our use-case is in a jailing SFTP, which we use the request-server to handle processing the requests by the clients differently.

We found that when the client requests '.', the library returns "/." (which is not a true absolute path) as the return, then Cyberduck pukes, and we have no way to recover.

Naturally, this used to work before with this library, so something changed within the SFTP library to cause this.

This is the log we got from Cyberduck, pointing to the use of the Realpath being the cause: https://gist.github.com/LordRalex/61b71bfac43232be8ef89068d2495e00

I have found the change which introduced this bug: https://github.com/pkg/sftp/commit/4d7bb970c49d8a86d12e2cd34952dc80dbc7cb0d#diff-412115c53c6c5fea203e8253f32d2645

In particular, this line of code was removed: https://github.com/pkg/sftp/commit/4d7bb970c49d8a86d12e2cd34952dc80dbc7cb0d#diff-412115c53c6c5fea203e8253f32d2645L186-189

Before: /. is transformed to / After: /. is returned to the client

This was a breaking change to the Cyberduck client, and so causes our application to therefore break.

Since /. is not a true absolute path, this code should be re-introduced.

I really wanted this to be a last resort, but I'm at my wits end trying to figure this out. My code is at the bottom. The main error I keep getting when using the typical configuration is "file does not exist." WinSCP has no issues.

Things I've tried:

1. Attempted the same operation using RClone to make sure my code is not the issue (see https://forum.rclone.org/t/sftp-remote-to-local-copy-failed-to-copy-file-does-not-exist/14414/2). RClone fails with the "File does not exist" error.

2. sftp.NewClient(conn) with sftpClient.Open(srcInvPath) - Received the error: "File does not exist"

3. sftp.NewClient(conn, sftp.MaxPacket(20480)) with sftpClient.OpenFile(srcInvPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC) - Received the error: "read from 13 for 20480 from 20497 not supported. (SSH_FX_FAILURE)"

4. sftp.NewClient(conn) with sftpClient.OpenFile(srcInvPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC) - Received the error: "read from 13 for 32768 from 32785 not supported. (SSH_FX_FAILURE)"

5. sftp.NewClient(conn) with sftpClient.OpenFile(srcInvPath, 0700) - Received the error : "EOF". Have no idea why I tried this.

package main

import (
	"bufio"
	"fmt"
	"io"
	"log"
	"os"
	"path"
	"path/filepath"
	"regexp"
	"strings"

	"github.com/pkg/sftp"
	"golang.org/x/crypto/ssh"
)

var invImpRootPath = "C:\\Invoices\\"
var invImpErrPath = filepath.Join(invImpRootPath, "Errors")
var invImpProcPath = filepath.Join(invImpRootPath, "Processed")
var invRegExp = regexp.MustCompile(`(?m)^(Invoice)\.([0-9]*)\.([0-9]*)\.([0-9]{1,5})\.xml$`)
var sftpInvPath = "/Invoices/outbound"

func main() {
	err := os.MkdirAll(invImpRootPath, os.ModeDir)
	if err != nil {
		fmt.Printf("Error creating Invoices directory: %s", err.Error())
		panic(err)
	}

	err = os.MkdirAll(invImpErrPath, os.ModeDir)
	if err != nil {
		fmt.Printf("Error creating Errors directory: %s", err.Error())
		panic(err)
	}

	err = os.MkdirAll(invImpProcPath, os.ModeDir)
	if err != nil {
		fmt.Printf("Error creating Processed directory: %s", err.Error())
		panic(err)
	}

	user := {removed}
	pass := {removed}
	host := {removed}
	port := "22"

	hostKey := getHostKey(host)
	config := &ssh.ClientConfig{
		User: user,
		Auth: []ssh.AuthMethod{
			ssh.Password(pass),
		},
		HostKeyCallback: ssh.FixedHostKey(hostKey),
	}

	conn, err := ssh.Dial("tcp", host+":"+port, config)
	if err != nil {
		log.Fatal(err)
	}
	defer conn.Close()

	sftpClient, err := sftp.NewClient(conn, sftp.MaxPacket(20480))
	if err != nil {
		log.Fatal(err)
	}
	defer sftpClient.Close()

	files, err := sftpClient.ReadDir(sftpInvPath)
	if err != nil {
		log.Fatal(err)
	}

	for _, file := range files {
		srcInvPath := path.Join(sftpInvPath, file.Name())
		dstInvPath := filepath.Join(invImpRootPath, file.Name())

		dstFile, err := os.Create(dstInvPath)
		if err != nil {
			log.Fatal(err)
		}
		defer dstFile.Close()

		srcFile, err := sftpClient.OpenFile(srcInvPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC)
		if err != nil {
			log.Fatal(err) 
		}

		bytes, err := io.Copy(dstFile, srcFile)
		if err != nil {
			log.Fatal(err)
		}
		fmt.Printf("%d bytes copied\n", bytes)

		err = dstFile.Sync()
		if err != nil {
			log.Fatal(err)
		}
	}
}

func getHostKey(host string) ssh.PublicKey {
	// parse OpenSSH known_hosts file
	// ssh or use ssh-keyscan to get initial key
	file, err := os.Open("known_hosts")
	if err != nil {
		log.Fatal(err)
	}
	defer file.Close()

	scanner := bufio.NewScanner(file)
	var hostKey ssh.PublicKey
	for scanner.Scan() {
		fields := strings.Split(scanner.Text(), " ")
		if len(fields) != 3 {
			continue
		}
		if strings.Contains(fields[0], host) {
			var err error
			hostKey, _, _, _, err = ssh.ParseAuthorizedKey(scanner.Bytes())
			if err != nil {
				log.Fatalf("error parsing %q: %v", fields[2], err)
			}
			break
		}
	}

	if hostKey == nil {
		log.Fatalf("no hostkey found for %s", host)
	}

	return hostKey
}

Server information from WinSCP:

Thank you!

In my project I would like to use github.com/pkg/sftp as an embedded SFTP server. Would be nice to make Server more extendable in terms of working with FS or having the following features:

1. Restrict user to use only specific directory (e.g. chroot)
2. Provide some kind of notifications when file upload is completed or user session is closed

In my pull request #94 I've moved all file operations into FileStorageBackend and made it replaceable. Maybe this solution is little bit dirty so let's workshop here the better solution

As i said in the title, I get "File does not exist" every time when using OpenFile. I can successfully call Open on the same file without any error, and am wondering what is causing this issue.

Thank you

The working directory now the sftp server gets is like "/D:/temp", but it can't be open on windows if I use "ls" command to list files under it. The root cause of it is the method of file request-server.go, the path.IsAbs(p) should be filepath.IsAbs(p), I tested it works fine. I don't know how to commit the code or create pull request. Please help to fix.

// Makes sure we have a clean POSIX (/) absolute path to work with func cleanPath(p string) string { p = filepath.ToSlash(p) if !path.IsAbs(p) { p = "/" + p } return path.Clean(p) }

This adds a hook just prior to handling the request packet. The request hook takes a reference to the request and returns an error and response packet. If the error is set the response packet is processed immediately and returned to the client.

Signed-off-by: David ML Brown Jr [email protected]

If we use pattern with trailing slash, like "/home/" and such object exist, than sftp.Glob duplicates object's name, and returns "/home/home". It happens because c.Lstat(pattern) finds existing object with trailing slash, but Split(pattern) doesn't split dir and object, and then Join(dir, file.Name()) joins duplicates.

func (c *Client) Glob(pattern string) (matches []string, err error) {
	if !hasMeta(pattern) {
		file, err := c.Lstat(pattern)
		if err != nil {
			return nil, nil
		}
		dir, _ := Split(pattern)
		dir = cleanGlobPath(dir)
		return []string{Join(dir, file.Name())}, nil
	}
....

I'am not sure how it should to work, but i think current logic not properly right. May i help?

I'm not sure if this is the same as the other "file upload hangs" issue.

On 1.13.5, randomly large uploads just stop uploading. This is with concurrent writes off, a 23mb file. I tried uploading the same file multiple times and it stopped at various places (12mb, 192kb), 5m later no further bytes had been added to the file on the target host. Out of two times I tried it in delve, one succeeded one failed (got stuck) at 12615680 bytes.

I tried with concurrent writes on and there was no issue (tried once).

This is the first time I've seen the issue. We do smaller file uploads fairly regularly (config files/small text files, probably <1kb). It's possible we've never done large uploads.

The code is almost identical to https://github.com/pkg/sftp/issues/502 but we have the whole source file in memory (using bytes.NewReader(data)).

I found the goroutine stuck at https://github.com/pkg/sftp/blob/v1.13.5/conn.go#L143 (admittedly it doesn't help much due to the async nature of the sftp code).

The destination server is also using this library (possibly a slightly older version).

`if f, err = s.client.Open(path.Join(remotePath, fileName)); err != nil { return err }

if err = os.MkdirAll(localPath, os.ModePerm); err != nil {
	return err
}
if localFile, err = os.Create(path.Join(localPath, fileName)); err != nil {
	return err
}
if n, err := f.WriteTo(localFile); err != nil {

return err } `

open file is ok, and i am sure that sftp server has the file but writeTo calling will return a err, err content is file does not exist

I create sshClient like this: if sshClient, err = ssh.Dial("tcp", addr, clientConfig); err != nil { return nil, err }; and sshClient is created successfully, after that create sftp client like this:

if sftpClient, err = sftp.NewClient(sshClient); err != nil { return nil, err } here I get an error "EOF" Please help this, where is a problam?