pkg / sftp

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

sftp.Client.Rename() giving permission denied, but works with other sftp clients #479

Open iamneal opened 2 years ago

iamneal commented 2 years ago

I have written some code that takes:

My code takes all this data, then:

my code works fine for reading the file from the server, but when it attempts to do a client.Rename(filename, archiveFilename) it is giving me:

sftp: "Failed to rename file /EXAMPLE_FILE 2021-11-02.csv" (SSH_FX_FAILURE)

similarly when I attempt to do client.Remove(filename) I am getting:

sftp: "User <redacted> does not have directory make, remove or rename permissions." (SSH_FX_PERMISSION_DENIED)

I thought maybe the sftp server didn't give our user enough permissions to move/delete files, but the rename worked with both command line sftp OpenSSH_8.1p1, and Filezilla when using the same user creds

The user permissions on the server allow us to read, put, and rename files in its userspace. It cannot mkdir, but the archive dir already exists. I can't give much more detail on the server architecture, because I don't have access, but if there is any other info I can provide, I'll try

my code look pretty much like this:

func processFile(file string, client *sftp.Client) error {
  f, err := client.Open(file)
  if err != nil {
    return err
  }
  readFile(f)

  f.Close()

  err = client.Rename(file, archiveFilename)
  if err != nil {
    logError(err)
    err = client.Remove(file)
    if err != nil {
      logError(err)
    }
  }
  return nil
}

I appreciate any help on this, thanks

puellanivis commented 2 years ago

I've been in and out of hospital all week, and have been without internet due to a recent move. Just wanted to give an FYI on this thread. I've been thinking about it, but haven't had much I can poke at.

puellanivis commented 2 years ago

I’ve been percolating on this for awhile. The only thing I can really think of is that the filenames are not being given as absolute? (i.e. sftp.Client does not naïvely support relative paths.) It’s hard to say without for instance example text from say OpenSSH_8.1p1. Seeing the actual commands issued, and the output resulting from it (obviously redacting secret information).

Understanding the full scope of what you’re trying to do as an example of sftp CLI commands can point to where assumptions may have been mistaken.

iamneal commented 2 years ago

Ok so update on this, I figured out the cause.

Our application copies a bunch of files from a sftp server, does some processing on them. This particular file was opened, copied, but only the WriteCloser we were writing to was closed. The remote file was left open. When we later attempted to do the sftpclient.Rename(file, archivefile), it failed because that reader was left open.

So basically the error was caused by doing this:

func example(client *sftp.Client, filename, archiveFilename string)
    readCloser, _ := client.Open(filename)  

    // defer readCloser.Close()  <-- didn't do this

    if err := client.Rename(filename, archiveFilename); err != nil {
        log.Warn(...) // <-- the error we logged
    }
}

The bug ended up being on our end, but its too bad we couldn't get a more descriptive error when we attempted to do the rename on the open file. Failed to rename file /EXAMPLE_FILE 2021-11-02.csv" (SSH_FX_FAILURE) is pretty generic.

Regardless, since the error was not in this package, we can close this issue. Thanks for the help

puellanivis commented 2 years ago

AAaaaaaah… yeah. Windows will do that sort of weird thing a lot. Lock a file basically and if you try and remove or delete it, it’s refused.

FYI: the defer readCloser.Close() probably wouldn’t help if it is in the same function call as a later Rename since the Close will not happen until after the Rename.

PS: On the error returned, the "Failed to rename file…” text comes from Windows itself, so there’s little we could even possibly do about improving the message. :(

iamneal commented 2 years ago

ah good catch on the example, that is a bit off.

So as far as improving the error message, it seems like the reason we can't improve it, is because that error is thrown from the remote end, so there isn't really a way to detect if the error was due to an open file, or if it was due to something else. is that right?

would it be possible to track which files are opened by an sftp.Client, and then throw an error if the user attempts a sftpclient.Rename() on a file that has been opened by that same client? At least for my application, there shouldn't be a reason to even attempt to rename a file if it is being read somewhere else.

This might be an optimization best left to the user of the sftp package though, and not in the sftp.Client

puellanivis commented 2 years ago

Yes, the SSH_FX_FAILURE error is a generic error code, with a free-form string returned as the message. So, there’s not really a way to unpack that error message programmatically and give a more meaningful information to the client.

I suppose in principle it would be possible to keep a map of open filenames, but the costs greatly outweigh the very narrow benefit that might be produced… and in particular, on *nix (Linux, Darwin, BSDs, etc) you can rename an open file. So, we would have to also OS-detect what the server is running on and… 🤪 it gets out of hand really quick.

So, yeah, unfortunately, there’s not much we can help here.