asyncssh icon indicating copy to clipboard operation
asyncssh copied to clipboard

Feature request: play nice with NamedTemporaryFile on Windows

Open hrichardlee opened this issue 1 year ago • 4 comments

I'd like to use asyncssh.scp with a file created via tempfile.NamedTemporaryFile, like:

async def write_to_file(
    connection: asyncssh.SSHClientConnection, bys: bytes, remote_path: str
) -> None:
    with tempfile.NamedTemporaryFile() as tmp:
        tmp.write(bys)
        tmp.flush()
        await asyncssh.scp(tmp.name, (connection, remote_path))

I think this is a fairly useful bit of functionality because it allows users to write text to a file without having to manage a local file themselves.

This works fine on Linux but not Windows. Based on this stackoverflow answer I believe this is because asyncssh.scp opens the local file without passing in the O_TEMPORARY flag.

I think the best options for implementing this are:

  • Allow passing in a file object in addition to a string representing the path (so let the user control how the file is opened)
  • Allow passing in a custom opener that will eventually be used by asyncssh.sftp.LocalFS.open

Thank you!

hrichardlee avatar Jul 12 '22 23:07 hrichardlee

This is an interesting idea, but I see a few challenges.

The main problem is that there are places in the code where stat() is called on the filename to get the file's size before a transfer starts. Calling stat() on the file object might be an option instead, but right now the copy code makes sure not to do that, to avoid an issue on some SFTP servers with single-use files (see #488 for details).

If the stat() was only done on local file objects, that issue might not apply, but one of the other challenges here is that the code is designed right now to abstract out whether files are local or remote as much as possible, making it difficult to special-case things. Perhaps that abstraction can be preserved, if I have LocalFS be the thing which handles open file objects, such that the open() call sort of turns into a no-op when a file object is passed in. I'd still need to go through all the places that a local srcpath or dstpath can be provided and change the type annotations to allow for this case, but that's something I could explore.

Another potential issue I see is that the code currently assumes that it can build the destination path from the source path, in cases where the original destination is a directory. If file objects are allowed, there won't be a way to get at what their original name was, which means a destination being a directory would not be supported, nor would things like the "recurse" flag.

In your case, is it a requirement to use 'scp', or would using SFTP be an option? With SFTP, you could just directly write the bytes to the file on the remote system, without the need to put them in a local file first.

ronf avatar Jul 14 '22 04:07 ronf

I work with @hrichardlee, just dropping in here - thanks for making this library, it's been working great for us.

In your case, is it a requirement to use 'scp', or would using SFTP be an option? With SFTP, you could just directly write the bytes to the file on the remote system, without the need to put them in a local file first

Interesting, I take it you mean by using https://asyncssh.readthedocs.io/en/latest/api.html#asyncssh.SFTPClient.open - I did miss that.

We could certainly explore that. I think it has a slight downside for us in that sometimes sftp service is not allowed by the target machine, and fixing that entails an additional step we were trying to avoid. It's not the end of the world though, but then neither is our current workaround.

kurtschelfthout avatar Jul 19 '22 17:07 kurtschelfthout

Welcome, @kurtschelfthout !

Yes - that was the API I was referring to. You're right that not all servers will support SFTP, but it's worth pointing out that OpenSSH 8.9's "scp" client actually now defaults to using SFTP to talk to the server, instead of launching a session and running "scp" on the remote system. While it's still possible to ask OpenSSH to use the old "scp" protocol for now, it has some security concerns that I think will make it slowly disappear in favor of SFTP.

ronf avatar Jul 20 '22 00:07 ronf

Ah, interesting, thanks. I'll give the SFTP API a try!

kurtschelfthout avatar Jul 20 '22 08:07 kurtschelfthout

Thanks for this @ronf, I'll close this issue as we're happy using SFTP.

hrichardlee avatar Aug 09 '22 20:08 hrichardlee

I keep forgetting to mention this, but also thank you for the amazing library.

hrichardlee avatar Aug 09 '22 20:08 hrichardlee

Happy to help, and that the library is working for you! Let me know if you run into any other issues.

ronf avatar Aug 10 '22 00:08 ronf