pkg / sftp

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

Support for the Afero interface #547

Open sudhirj opened 1 year ago

sudhirj commented 1 year ago

I'd like to run an SFTP server with virtual content, and was thinking it would be much easier to support the Afero filesystem instead of or in conjunction with the existing handler interfaces.

https://github.com/spf13/afero has the Afero interface, which is widely used in the community, including in https://github.com/fclairamb/ftpserverlib and many other projects.

Supporting Afero will also indirectly support loading the standard library's fs.FS as an SFTP server using the Afero adapter.

If we want this I can arrange for a PR.

puellanivis commented 1 year ago

Since the afero.Afero interface uses afero.File types, I’m not sure we want to implement it. We’ve already implemented the github.com/kr/fs.FileSystem interface, which has basically been superseded by the io/fs.FS interface. I would view the implementation of the kr/fs.FileSystem as a mistake, as it put an unnecessary external dependency on the package.

I don’t think anything is wrong with the interface they have, but since we would have to import the package in order to implement the interface, 😬 I’m wary of pulling in Yet Another File System Interface Abstraction Interface.

Instead, I would implement something similar but with our own File type, and then anyone who is interested, can easily implement a compatibility layer, by converting our Client calls to the Afero package interface. This would be the most appropriate path forward for what you’re suggesting.

sudhirj commented 1 year ago

How about implementing the handlers on fs.FS? That would be a read-only system, but do you think it would be appropriate to upstream?

We could provide a way to wrap that with write handlers on the existing interface.

puellanivis commented 1 year ago

Yes, implementing fs.FS is the interface that we should be targetting as an exportable interface.

Ideally, Client should have a receiver method FS() (io/fs.FS, error) or maybe even just Sub(dir string) (io/fs.FS, error) directly, that would fit the interface. It would have to be a wrapper, and it should only provide the functionality that io/fs.FS expects, that is, it would be read-only. If someone needs write handlers, then they would need their own wrapper to meet whatever interface they’re using.

Implementing more than what io/fs.FS expects, like providing say Create(name string) (io.WriteCloser, error) is fraught with issues, in that if io/fs ever does provide a definitive CreateFS and we’re not meeting that interface, we couldn’t add a correct one, because it would collide with the Create we already implement.

P.S. part of why I haven’t continued my work on the Start using filexfer PR, is that we’re really at a point here, where we need to implement a v2.0.0 of the API, and clean up decisions made in the past in the name of expedience to provide a feature, but which ended up bringing us out of alignment with everyone else. As an example: OpenFile doesn’t take a perm fs.FileMode.

sudhirj commented 1 year ago

Would it make sense then to do a simpler server API as well? Seems like a lot of boilerplate when just an auth handler and FS are enough to run a full server.

puellanivis commented 1 year ago

Yeah, there’s a few design ideas there that could simplify a lot.