request server: add support for SSH_FXP_FSETSTAT

[drakkan]

we need to add a case for this packet inside the packet worker otherwise
it will be handled in hasHandle case and it will become a "Put" request.

Client side if a Truncate request is called on the open file we should
send a FSETSTAT packet, the request is on the handle, and not a SETSTAT
packet that should be used for paths and not for handle.
We should probably do the same for Chmod/Chtimes/Stat too.

This patch fix file corruption using sshfs. Simply create a text file using sshfs and then delete some lines, the file will be corrupted.

See the truncate issue reported here for more details

[puellanivis]

Hm… interesting. Is this something really critical that we should address before releasing 1.12? File corrupt does seems like a showstopper.

[drakkan]

Hm… interesting. Is this something really critical that we should address before releasing 1.12? File corrupt does seems like a showstopper.

It depends on what the client do. We will have corrupted files if the client use SSH_FXP_FSETSTAT to truncate the file. But we have corrupted files also if the client read and write to the same file, see #374

Probably we should also add an optional Truncate interface and call it on the aleady opened file, for example in SFTPGo I have to do something like this because if a issue an os.Truncate on an already opened file I get errors on some OS (for example on Windows). Anyway I can handle this in SFTPGo. Please let me know what do you think about, thank you

[drakkan]

Hi,

I improved the patch. After a Truncate the offset for the next read is now updated as expected.

I was wrong about os.Truncate it works with the file already opened even on Windows so we don't need another interface

[drakkan]

Regarding CI I updated travis to go 1.15 however I would switch to GitHub actions in future, Actions are better integrated with GitHub and we cannot merge a pull request without noticing that it breaks build, do you have objections?

Do you think we can break compatibility after 1.12? I would like to do the following backward incompatible changes:

• don't hide Lstat: currently the request server call Stat for both sshFxpLstat, sshFxpStat and sshFxpFstat see here
• remove Put and Get methods and add a generic OpenFile method. This should fix file corruption if a client try to read and write to the same handle. Do you think this should be fixed before 1.12?
• add support for golangci-lint (as GitHub action) and so remove all the current warnings
• maybe remove support for the old server mode and leave request-server only

[puellanivis]

If you want to do the work to switch us over to GitHub actions over travis, 🤷‍♀️ I wouldn‌’t complain 😆

I’m honestly not all that impressed with golangci-lint. If we want to clean up the code, then we can manually run any linting we want and resolve any raised issues when the make sense. Enforcing a list of linting to pass in order to greenlight a PR looks good on paper (which is why it shows up so often in corporate codebases), but it is actually a nightmare of appeasing arbitrary and often poorly considered rules. (In particular, golangci-lint seems to be focused on embedding via copy-paste All The Linters, rather than focusing on targetting a well defined and scoped Best Practice.)

As for reworking the whole RequestServer mechanics, I’m kind of 😬 about. The only reason to have RequestServer is that it is a helpful abstraction layer hiding facets of SFTP from the implementing service. If the only thing it does is thinly wrap the whole of the SFTP protocol, then it stops being all that useful. If suddenly now one has to implement all the features to get any functionality at all? 👎 More ideally, we want to give a quick access to 80% of the functionality without excessive overhead. If someone needs that 20% we don’t support, then they can deal with a more primitive layer directly (that perhaps we could also build out and expose as a subpackage).

[drakkan]

If you want to do the work to switch us over to GitHub actions over travis, I wouldn‌’t complain

Ok I'll submit a pull request

I’m honestly not all that impressed with golangci-lint. If we want to clean up the code, then we can manually run any linting we want and resolve any raised issues when the make sense. Enforcing a list of linting to pass in order to greenlight a PR looks good on paper (which is why it shows up so often in corporate codebases), but it is actually a nightmare of appeasing arbitrary and often poorly considered rules. (In particular, golangci-lint seems to be focused on embedding via copy-paste All The Linters, rather than focusing on targetting a well defined and scoped Best Practice.)

Ok, we will discuss better about this in future, I'll try to send a pull request with some basic linters

As for reworking the whole RequestServer mechanics, I’m kind of about. The only reason to have RequestServer is that it is a helpful abstraction layer hiding facets of SFTP from the implementing service. If the only thing it does is thinly wrap the whole of the SFTP protocol, then it stops being all that useful. If suddenly now one has to implement all the features to get any functionality at all? More ideally, we want to give a quick access to 80% of the functionality without excessive overhead. If someone needs that 20% we don’t support, then they can deal with a more primitive layer directly (that perhaps we could also build out and expose as a subpackage).

Request-Server is the only choice if you want to interact with non local fs (in SFTPGo I support S3 and GCS and I could add Azure blog storage in future).

I think we can add optional interfaces for the missing features instead of a new subpackage. I need the generic openfile method to fix the reported file corruption issue in SFTPGo. What do you think about an optional interface? If it is implemented it will be called instead of "Get" and "Put", we can do the same for "Lstat" if an optional interface is implemented we call it otherwise we call "Stat" as now.

I don't know the history of this project, based on your answer I understand that the Request-Server is not intented to replace the server implementation. OK.

[puellanivis]

What do you think about an optional interface?

Yeah, this is a good idea. We should probably work on converging towards something similar to the io/fs proposal that has been proposed, and the coolest idea I think to come from that proposal is the idea of extension interfaces providing optional support for features.

Exactly like you said, this would be nice since a “simplest” implementation of Lstat/Stat would be one function doing Stat, but maybe you need and/or can provide an Lstat then we could then use implementation of the extension interface to know we could do a specific Lstat instead of just the same Stat.

The project wasn’t probably as well defined of scope when it started unfortunately. And I think most of the API popped in without a multi-draft proposal process, so some choices were made that were maybe not ideal. (I know pkg/errors suffers from some legacy choices that are too late to change now without significant breakage, and the breaking changes would be of little benefit since most of the package’s functionality has been replaced by fmt.Errorf("… %w").)

What with the (probable) future roll out of io/fs proposal, that might provide us with a good opportunity to rethink how this whole package should work and what it should look like, and then we could run a few drafts of a v2, and make a solid break towards a better implementation overall, rather than piecemeal breaking changings.

[drakkan]

What do you think about an optional interface?

Yeah, this is a good idea. We should probably work on converging towards something similar to the io/fs proposal that has been proposed, and the coolest idea I think to come from that proposal is the idea of extension interfaces providing optional support for features.

Ok, I'll try to send a new patch for this (later this week or the next one). Thank you for your reviews :-)

Exactly like you said, this would be nice since a “simplest” implementation of Lstat/Stat would be one function doing Stat, but maybe you need and/or can provide an Lstat then we could then use implementation of the extension interface to know we could do a specific Lstat instead of just the same Stat.

The project wasn’t probably as well defined of scope when it started unfortunately. And I think most of the API popped in without a multi-draft proposal process, so some choices were made that were maybe not ideal. (I know pkg/errors suffers from some legacy choices that are too late to change now without significant breakage, and the breaking changes would be of little benefit since most of the package’s functionality has been replaced by fmt.Errorf("… %w").)

What with the (probable) future roll out of io/fs proposal, that might provide us with a good opportunity to rethink how this whole package should work and what it should look like, and then we could run a few drafts of a v2, and make a solid break towards a better implementation overall, rather than piecemeal breaking changings.

I quicky readed the proposal some weeks ago, I'll read it better and in future I'll try to help with v2

[puellanivis]

I don’t think we need a new patch right now. And most of this patch is already pretty necessary to avoid calling a Put request on something that is definitely not a Put.

We can get this done, and hopefully checked in before EOW, then cut a release for Friday? I hope? And then we can start looking for the future on how we want a v2 to look.

[drakkan]

I don’t think we need a new patch right now. And most of this patch is already pretty necessary to avoid calling a Put request on something that is definitely not a Put.

Hi, I'm not fully understanding your answer. We have two issues here:

1. an SSH_FXP_FSETSTAT that will become a Put request. This issue is fixed in this pull request and if the test cases are now ok I can merge it myself just now
2. if a client open a file for both reading and writing the code here will set the request method as Put and if the client try to read it will corrupt the opened file. We discussed about this issue in Request server: allow to open a file for both reading and writing #374. It is not clear to me if we want to fix this issue before or after 1.12. If we can break compatibility the fix is quite easy otherwise we should try to add an optional interface and document that if it is not implemented we can have a file corruption issue.

I can work on this today, I'll will be unavailable from tomorrow until Friday

We can get this done, and hopefully checked in before EOW, then cut a release for Friday? I hope? And then we can start looking for the future on how we want a v2 to look.

[puellanivis]

1. Yes, we want this fixed, and the tests are good now. You can merge it. Then we should be good for 1.12. (I will probably do a go module update just before release tomorrow.)

2. Yeah, we need to think about this a bit more than we properly have time for right now. Ideally, we need a better design over all, which better considers this possible case from the ground up. We’ll work on a possible v2 design and iterate on the ideas and see where we go from there, sound good?

[drakkan]

1. Yes, we want this fixed, and the tests are good now. You can merge it. Then we should be good for 1.12. (I will probably do a go module update just before release tomorrow.)

ok

2. Yeah, we need to think about this a bit more than we properly have time for right now. Ideally, we need a better design over all, which better considers this possible case from the ground up. We’ll work on a possible v2 design and iterate on the ideas and see where we go from there, sound good?

sounds good