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 7bf7a25 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!

[puellanivis]

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.

[puellanivis]

We should not be defaulting this value. Please, pass it as a parameter, and document in the godoc that it is ignored if the creation flag is not set.

[zelch]

Fair enough, I had defaulted to the value that we previously hard coded. I'll take a crack at getting it from the initial request.

[puellanivis]

This function also now needs to that directories might have non-default modes.

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

[puellanivis]

Memfile is intended to be a stripped-feature POSIX compatible filesystem.

It turns out, this behavior is a bug. It just wasn’t apparent, because I wasn’t implementing permissions on the filesystem, and we do not stress test this package for POSIX compatibility.

Any feature expansion to support permission should comply with POSIX compatibility of permissions, and this includes directories having permissions, and the behavior of those permissions.

When files don’t have permissions, it does not violate the principle of least astonishment that directories do not have permissions as well. But if files have permissions, it does violate the principle of least astonishment that directories do not have permissions.

[zelch]

This one is awkward, and I think that we might be better off documenting where we are not POSIX compliant.

Otherwise we're left checking the permissions of every directory under a given file. That wouldn't necessarily be the end of the world, but it definitely makes all the file open cases more complicated.

[puellanivis]

Fortunately, opening both files and directories goes through either fetch or lfetch.

And yeah. :( POSIX is really hard, but our testing code kind of relies upon it being as POSIX-compliant that we can get. Last I touched this, I basically had to do a complete overhaul to correctly support open flags properly. … 🤔 I think I might have avoided working on permissions and ownership precisely because things were getting out-of-hand, and it would require an even further restructure/refactor to more properly emulate inodes in their entirety.

[zelch]

Fortunately, this is all still passing the testing infrastructure, so I'm still very inclined to just keep permissions on directories as 'unimplemented' for right this moment.

[puellanivis]

It’s passing the testing infrastructure merely because we’re not sufficiently testing it. :blobsweat:

I think if we’re going to start implementing permissions for files, but not for directories, then we’re going to need to start documenting what is covered and what is not. Right now attempting to change permissions on anything won’t do anything, and users are likely to say, “oh… I guess no permissions?” (Somewhat typical situation because of Windows.) Or changing ownership yields, “oh, I guess no ownership.”

But “wait, but file permissions work, and dir permissions don’t?!” is going to lead to questions that we can answer with documentation. If you’re not willing to put in the effort of :blobsweat: implementing the full permissions system. (I don’t blame you. I clearly stopped short.)

[zelch]

The problem with implementing directory permissions is that I can't even remember the esoteric rules on what happens when one of read or execute is set without the other set, let alone the special bits. Especially since we don't even have a chdir at that level, just get of specific full paths. And I definitely recall that it gets weird when you have that be the case for an intermediate directory in the path, but you have full permissions for a subdirectory.

Only, now you've made me look at the POSIX specification with a migraine, ugh.

Writing test cases for this is going to be painful, but I'll take a look in my free time. Sadly, having gotten stuff working 'well enough' for the work application, it's harder to spend work time on the remaining bits.

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

[puellanivis]

😭 Now, I think I know why I stopped before implementing permissions.

[puellanivis]

And… 🤦‍♀️ we run into another bug of design.

The problem here is that we’re going to need to Marshal flags itself based on if ls(fs.Extended) is set or not. 😐

This split implementation is really going to drive me nuts, and might just drive me mad enough to go ahead and just roll out v2…

[puellanivis]

See? Think about what if flags & sshFileXferAttrExtended == sshFileXferAttrExtended and len(fileStat.Extended) == 0.

We’re going to encode a corrupted packet. 🤦‍♀️

[zelch]

Thankfully, that's not actually possible, but why it's not possible is subtle:

    // if fi implements FileInfoExtendedData, retrieve extended data from it
    if fiExt, ok := fi.(FileInfoExtendedData); ok {
        fileStat.Extended = fiExt.Extended()
        if len(fileStat.Extended) > 0 {
            flags |= sshFileXferAttrExtended
        }
    }

That's from fileStatFromInfo.

So it's not possible to have sshFileXferAttrExtended set while len(fileStat.Extended) == 0.

[puellanivis]

Yes, for marshalFileInfo this might be true, but since FileStat.MarshalTo would be exported, more people can call it than just this code here, and the RequestServer code as well.

[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?