Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SFTP Test Server #505

Open
c4r3 opened this issue May 14, 2022 · 20 comments
Open

SFTP Test Server #505

c4r3 opened this issue May 14, 2022 · 20 comments

Comments

@c4r3
Copy link

c4r3 commented May 14, 2022

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
Copy link
Collaborator

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

@c4r3
Copy link
Author

c4r3 commented May 14, 2022

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
Copy link
Collaborator

drakkan commented May 14, 2022

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
Copy link
Author

c4r3 commented May 15, 2022

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
Copy link
Collaborator

drakkan commented May 15, 2022

@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
Copy link
Collaborator

//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
Copy link
Author

c4r3 commented May 16, 2022

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
Copy link
Collaborator

puellanivis commented May 16, 2022

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
Copy link
Author

c4r3 commented May 16, 2022

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
Copy link
Collaborator

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

@c4r3
Copy link
Author

c4r3 commented May 16, 2022

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
Copy link
Collaborator

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

@c4r3
Copy link
Author

c4r3 commented May 16, 2022

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

  • accepted: true
    `

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
Copy link
Collaborator

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
Copy link
Author

c4r3 commented May 16, 2022

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
Copy link
Collaborator

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

@drakkan
Copy link
Collaborator

drakkan commented May 16, 2022

@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
Copy link
Author

c4r3 commented May 16, 2022

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

@c4r3
Copy link
Author

c4r3 commented May 16, 2022

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
Copy link
Collaborator

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. 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants