Memfile stuff

[zelch]

There are roughly three sets of changes in this PR, all in the memfile backend.

The first set is using errors.Is to check error types, and this is primarily to make the linter happy. (What can I say, if I'm working on a file, it's much easier to just make it lint cleanly before I do anything else.)

The second is making Setstat more functional: Support for setting the mode and ctime, which involves tracking the file mode.

And the third is making it practical to use Setstat on a memfile backend directly, for the purpose of letting test frameworks do things like set the mode and ModTime on test files.

The commit message on 7bf7a25d28a84d6862c0d21c5013d84f315bd459 spells out why I'm not especially happy with the solution I came up with.

It works, it's minimally invasive, and it just feels wrong.

I'm very open to alternate solutions here. A function to construct a Request object with attributes also feels somewhat messy, especially without defining a better structure format to use at some point.

[zelch]

And I went ahead and wrote up an alternate option, with the badly named SetAttributes allowing us to construct a valid SetAttr Request object.

Again, suggestions on how this could be improved are more than welcome!

[zelch]

As well as the comments inline, there are a few other cases where err is compared directly against os.ErrNotExist. It seems your linter is only catching err != os.ErrNotExist and not err == os.ErrNotExist?

Otherwise, this seems pretty good so far.

Good catch, and while I'm at it, I should probably tackle all of the other cases of this as well, not just those in request-example.go.

[zelch]

Hi @puellanivis, based on your feedback I redesigned the entire process. (And did a few other things.)

I'm still not entirely satisfied by the design, but I'm not sure that it's going to be possible to do better without causing an API break.

[zelch]

Alright, I think that this version is a lot cleaner, if not perfect.

As a matter of preference, do you want a PR without all of the history of the various different designs that were discarded, or would you prefer to leave it as a record of things not to do?

[urko-b]

Hi, I don't know if this stuff I want to share is out of scope but I did an implementation of a in memory map[string]custom.Bytes struct which handles storing files in memory and uploading them to some storage/local etc...

THen I did another version which is writting files into temp directory. I used sftp.NewRequestServer()

func newWriteOnlyHandler(tempDir string) *writeOnlyHandler {
    return &writeOnlyHandler{
        tempDir: tempDir,
        files:   make(map[string]string, 0),
    }
}

type writeOnlyHandler struct {
    tempDir string
    files   map[string]string
}

func (ch *writeOnlyHandler) Filewrite(r *sftp.Request) (io.WriterAt, error) {
    localPath, ok := ch.files[r.Filepath]
    if !ok {
        localPath = filepath.Join(ch.tempDir, filepath.Base(r.Filepath))
        ch.files[r.Filepath] = localPath
    }
    file, err := os.OpenFile(localPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o666)
    if err != nil {
        return nil, err
    }
    // Do not need to close as we delegate this to sftp server
    return file, nil
}

var errOperationNotAllowed = errors.New("this operation is not allowed")

// Deny read requests since we only want to allow write (put) operations.
func (ch *writeOnlyHandler) Fileread(r *sftp.Request) (io.ReaderAt, error) {
    return nil, errOperationNotAllowed
}

// Deny read requests since we only want to allow write (put) operations.
func (ch *writeOnlyHandler) Filecmd(r *sftp.Request) error {
    return errOperationNotAllowed
}

// Deny read requests since we only want to allow write (put) operations.
func (ch *writeOnlyHandler) Filelist(r *sftp.Request) (sftp.ListerAt, error) {
    return nil, errOperationNotAllowed
}

Then I used it like this:

handlers := sftp.Handlers{
            FileGet:  s.writeonlyHdl,
            FilePut:  s.writeonlyHdl,
            FileCmd:  s.writeonlyHdl,
            FileList: s.writeonlyHdl,
        }
        reqServer := sftp.NewRequestServer(channel, handlers, sftp.WithStartDirectory(tempdir))

[puellanivis]

Thanks for your input @urko-b . I think our earlier versions of InMemoryHandler approached things in a very similar manner. The problem we ran into is that it was acting unpredictably from what expectations of how a POSIX filesystem should work, and things… uh… snowballed. As I think you can imagine.

[urko-b]

Thanks for your input @urko-b . I think our earlier versions of InMemoryHandler approached things in a very similar manner. The problem we ran into is that it was acting unpredictably from what expectations of how a POSIX filesystem should work, and things… uh… snowballed. As I think you can imagine.

OK, I'm not an expert about the POSIX, even have quite knowledge :D BTW this example I've shared is working good on ubuntu VPS server. So If you want me to add an example that is working I could prepare a PR

[zelch]

func (ch *writeOnlyHandler) Filewrite(r *sftp.Request) (io.WriterAt, error) {
  localPath, ok := ch.files[r.Filepath]
  if !ok {
      localPath = filepath.Join(ch.tempDir, filepath.Base(r.Filepath))
      ch.files[r.Filepath] = localPath
  }
  file, err := os.OpenFile(localPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o666)
  if err != nil {
      return nil, err
  }

Then I used it like this:

handlers := sftp.Handlers{
          FileGet:  s.writeonlyHdl,
          FilePut:  s.writeonlyHdl,
          FileCmd:  s.writeonlyHdl,
          FileList: s.writeonlyHdl,
      }
      reqServer := sftp.NewRequestServer(channel, handlers, sftp.WithStartDirectory(tempdir))

Hm, how are you avoiding directory traversal attacks?

That is, assuming that ch.tempDir is /home/sftp_user/tmp, and tempdir is /, what's to stop someone from doing a put to /../.bash_profile?

[puellanivis]

Again, thanks for the input @urko-b but there’s like so many caveats and gotchas that come into play here. Subtle loopholes of the standards, etc.

I would really rather not introduce yet another simplified implementation of an in-memory filesystem. We’re already struggling to cover the existing one, as can be demoed by the length of this PR.

[puellanivis]

Just checking back in. 🤔 I think the only hang up here was on if we wanted to also develop directory permissions?