pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.5k stars 379 forks source link

SFTP Test Server #505

Open c4r3 opened 2 years ago

c4r3 commented 2 years ago

I created a simple SFTP Test server to achive some assertions with my unittest of my SFTP client. I start the server in a dedicated go routine at the startup, then i execute all my unittest and i close it on teardown phase. The connection is established, but i had an issue during the upload of the file: when the client "open" the remote file i had an error of "connection lost". There are few lines of code involved:

dstFile, err := client.OpenFile(remoteFilePath, (os.O_WRONLY | os.O_CREATE | os.O_TRUNC)) if err != nil { return fmt.Errorf("unable to open remote file: %v", err) }

The error is: "unable to open remote file: connection lost".

Due to this issue i created a new dedicated project with just the SFTP server (the same of the sftp/examples/go-sftp-server/) and i used CyberDuck to test the uploading of the file. Cyberduck can connect correctly to the server but when i try to upload a file the connection hangs, nothing happens and it terminates with timeout.

I'm not understand what's wrong, any ideas?

puellanivis commented 2 years ago

We probably need more source code to figure out what could be going on.

c4r3 commented 2 years ago

Sure. I just copy-and-pasted the source code of "go-sftp-server" in a new Golang project and i executed it with "go run ...". CyberDuck established correctly the connection and then hangs when i try to upload just a small txt sample file. On the other side my SFTP upload method is simple:

`func upload(client *sftp.Client, localFilePath string, remoteFilePath string) error {

//Closing client after all activities
defer client.Close()

logs.Logger.Debugf("Uploading [%s] to [%s]", localFilePath, remoteFilePath)

srcFile, err := os.Open(localFilePath)
if err != nil {
    return fmt.Errorf("unable to open local file: %v", err)
}
defer srcFile.Close()

logs.Logger.Debugf("Remote file path: %s", remoteFilePath)

// Note: SFTP Go doesn't support O_RDWR mode
dstFile, err := client.OpenFile(remoteFilePath, (os.O_WRONLY | os.O_CREATE | os.O_TRUNC))
if err != nil {
    return fmt.Errorf("unable to open remote file: %v", err)
}

defer dstFile.Close()

bytes, err := io.Copy(dstFile, srcFile)
if err != nil {
    return fmt.Errorf("unable to upload local file: %v", err)
}

logs.Logger.Debugf("%d bytes copied", bytes)

return nil

}` The SFTP Client is feeded by reference just to separate the business logic of the creation of the client and the uploading one.

drakkan commented 2 years ago

Hi,

I think the example sftp server only accepts/serves a single connection. This could be the issue.

In your sample code I read:

Note: SFTP Go doesn't support O_RDWR mode

I'm not sure about the server implementation, but if you use the request-server it works fine (I added this feature a while ago), you can easily check this feature using SFTPGo

c4r3 commented 2 years ago

Hi @drakkan, first of all the note is into the upload method and not into the server file. By the way i don't think it's a problem because is just about that specific syscall. I tested the two implementation of SFTP server written in Go available into the "example" folder of this project. Both of them have the same issue: the upload hangs and nothing works. I tested the two implementations with CyberDuck just to understand if the problem is with the implementation of the server or my upload business logic. I'll check the implementation you highlighted as soon as possible. I even didn't understand the differences between the two implementations of the servers into the example folder.

drakkan commented 2 years ago

@c4r3 the examples only handle a single connection, if you connect twice the second connection will not work. I guess this is the problem when you connect with CyberDuck (I've never used it but FileZilla, for example, opens a new connection when you upload a file).

The server implementation is a ready to use implementation that uses the local filesystem, the request server allows to implement your custom logic.

SFTPGo is a full-featured SFTP server that uses request server implementation, handles multiple connections, storage backends other than the local filesystem etc. It's probably overkill for your use case, but by testing with it you can figure out what the library really allows.

puellanivis commented 2 years ago
//Closing client after all activities
defer client.Close()

Normally, you would not put this in an upload command, you would put this in the wider scope where you created the client.

c4r3 commented 2 years ago

Hi @puellanivis, you are right but i choose to close the client at the end of the upload command. I prefer a complete separation between the client creation business logic and the opations one (CRUD operations). In this case i have to close the client to avoid leak.

puellanivis commented 2 years ago

I prefer a complete separation between the client creation business logic and the opations one (CRUD operations).

In that case, you’ve made a mistake. The client.Close() is part of the business logic of the client creation. There is no reason to be closing the client after every upload. This could even be causing your problem, because if you start up the server, then make a client connection, upload, close the client, then try and upload again, well… the client is already closed, so connection lost.

In this case i have to close the client to avoid leak

The client needs to stay open for the entirety of your operation business logic. You only client.Close() at the end when all of your operations business logic is complete. It is not a “leak” to have this connect standing open while you’re doing operations on it. So, unless every call to this upload function is preceded by a sftp.NewClient() call, then you’re misbehaving here, and even if you were doing that, you should have the defer client.Close() following the sftp.NewClient() if err != nil { return nil, err } block, not in a subfunction separated from that client creation, because as mentioned above, this client.Close() call is part of the client creation business logic.

c4r3 commented 2 years ago

I agree but i omitted one detail: it's just the first implementation, i'll share the same instance of the client to avoid "opening and closing" each time. By the way i have a "connection lost" error even the first time (i'm sure the client is up and running). Furthermore as i wrote i have the same issue with CyberDuck (so my Upload method is out of scope here).

Maybe the issue is due to the capability of the server to manage just a connection, i have to test it.

puellanivis commented 2 years ago

I’m a little confused now… if that is out-of-scope, then we need to see your server code, right?

c4r3 commented 2 years ago

When i had the first "connection lost" of my upload method, i started to check if the SFTP test implementation worked fine. The server code is the same of the one into the "example" package. I just copy and paste the same file into a new Golang project, i run it and test it with CyberDuck. Maybe even my upload business logic has a bug, but i must validate the server at first.

puellanivis commented 2 years ago

Do you have any of the output logs from the server when it is running?

c4r3 commented 2 years ago

Sure, this is the stdout in my shell (i enabled the debug mode with the corresponding flag).

` sftp_test_server go run main.go Listening on [::]:8022 Login: user 2022/05/16 11:44:33 login detected: user SSH server established Incoming channel: session Channel accepted Request: subsystem Subsystem: sftp

As you can see the last log message is "accepted: true" into the for loop on the chan of incoming ssh requests. In CyberDuck i get the popup of the public key of the server and the connection establishment completed successfully.

puellanivis commented 2 years ago

Can you try with the OpenSSH sftp client? Everything I’m trying here seems to still work.

Plus, I see that you’re connecting with user instead of testuser is this the only change you made? Even subtle changes could be breaking things?

c4r3 commented 2 years ago

Yeah sorry, i just changed the defauuser e password. I'll check with OpenSSH as soon as possibile. I'm wondering why i have no other errors or log messages if it's due to that.

puellanivis commented 2 years ago

Yeah, this is turning into something really weird, as I’m unable to reproduce.

drakkan commented 2 years ago

@puellanivis if you start the example server and you connect with FileZilla the connection will work and FileZilla will list the files. If you try to upload a file you will get something like this:

Risposta:   fzSftp started, protocol_version=11
Comando:    open "testuser@127.0.0.1" 2022
Errore: Timeout connessione dopo 20 secondi di inattività

this happens because the example code just handle a single connection and filezilla tries to open a new connection to upload the file. I guess Cuberduck works the same way

c4r3 commented 2 years ago

i agree. Thank you guys for all. I precisely started with the idea of avoid this kind of issues in production.

c4r3 commented 2 years ago

I had no time this evening to do other testing, so i have no updates. @puellanivis have you tried CyberDuck to reproduce the issue?

puellanivis commented 2 years ago

No, I haven’t tried CyberDuck myself. 🤔 I hate to install all the various SFTP client applications, but I suppose its probably inevitable at some point. 🤣