Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server.go: "/" for windows #571

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 51 additions & 5 deletions server.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"errors"
"fmt"
"io"
"io/fs"
"io/ioutil"
"os"
"path/filepath"
Expand All @@ -21,6 +22,51 @@ const (
SftpServerWorkerCount = 8
)

type file interface {
Stat() (os.FileInfo, error)
ReadAt(b []byte, off int64) (int, error)
WriteAt(b []byte, off int64) (int, error)
Readdir(int) ([]os.FileInfo, error)
Name() string
Truncate(int64) error
Chmod(mode fs.FileMode) error
Chown(uid, gid int) error
Close() error
}

type dummyFile struct {
}

func (f *dummyFile) Stat() (os.FileInfo, error) {
return nil, os.ErrPermission
}
func (f *dummyFile) ReadAt(b []byte, off int64) (int, error) {
return 0, os.ErrPermission
}
func (f *dummyFile) WriteAt(b []byte, off int64) (int, error) {
return 0, os.ErrPermission
}
func (f *dummyFile) Readdir(int) ([]os.FileInfo, error) {
return nil, os.ErrPermission
}
func (f *dummyFile) Name() string {
return "dummyFile"
}
func (f *dummyFile) Truncate(int64) error {
return os.ErrPermission
}
func (f *dummyFile) Chmod(mode fs.FileMode) error {
return os.ErrPermission
}
func (f *dummyFile) Chown(uid, gid int) error {
return os.ErrPermission
}
func (f *dummyFile) Close() error {
return os.ErrPermission
}

var _ = dummyFile{} // ignore unused
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have made your life significantly harder than it needs to be.

We only have two real implementations: os.File and our internal pseudo-directory listing the drives.

You have however, here implemented an abstraction layer to allow our internal pseudo-directory listing to just embed this and voila, it is now automagically a FileLike. Except, we don’t need that magic. We can just implement all of this directly on WinRoot directly. Then we only have the one type rather than two.

This also solves the problem you’ve created here where dummyFile.Name() always returns "dummyFile" which is a valid filename, and quite an inappropriate value just be returning like everything should automatically know that it’s not a real filename.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think this however makes it super easy to extend the current codebase for more "virtual paths", for which only "a small subset of file operations" is implemented. One can now just "inherit" dummyFile (via promoted fields) and "overwrite" the methods one needs to implement.

This also solves the problem you’ve created here where dummyFile.Name() always returns "dummyFile" which is a valid filename

Or one just removes "Name()" from the list of functions a default implementation is provided? What filename do you suggest?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you know? It doesn't add much complexity IMO. It allows one to do interesting stuff, for example one could create a /proc like "filesystem" that is completely implemented in the sftp server! I surely could see some interesting use cases for that, which I might even add later (in a private fork). I think one should consider the pros and cons, I don't see many cons in this case? But maybe I am missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to add it to your private fork, I cannot stop you. But we don’t need this code in this package.

Copy link
Author

@powellnorma powellnorma Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixing dummyFile with winRoot IMO mixes two separate issues.. The first is to implement a "dummy" that realizes the file interface, the second is to implement the winRoot functionality.

When one looks at winRoot, one imediatly sees the gist of what it is doing. It is not obscurred by all the "dummy implementations", because those are contained in dummyFile.

But we don’t need this code in this package.

Who is "we"? Do you know who is going to fork it, and potentially want to add functionality? I don't think so.

YAGNI

I think this principle is useful for deciding if one wants to invest the time into implementing something.. But in this case it is already implemented, and IMO is cleaner (for the reason described above).

If this gets into "No you have to change X and Y and Z because I like it more" without mentioning real reasons behind that, then I must tell you that I don't have time for that. It is similar to the "No you need to use permission denied, because I say so", and disregarding the point I gave on why I think Operations cannot be performed on this handle is more matching for a situation where not the privilege level of the user is the problem, but that the operation makes no sense for the referenced resource.

This is your repository so you are free to do what you want. But don't expect me to change this while not giving any clear reasons, thanks 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why SSH_FX_PERMISSION_DENIED?

https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03

[EPERM]
Operation not permitted. An attempt was made to perform an operation limited to processes with
appropriate privileges or to the owner of a file or other resource.

Consult https://elixir.bootlin.com/linux/latest/C/ident/EPERM and look for basically any use therein. There are far more uses of EPERM for “Operation not permitted” rather than simply “file permissions do not allow this operation”.

For example: https://man7.org/linux/man-pages/man2/mkdir.2.html

EPERM  The filesystem containing pathname does not support the creation of directories.

Clients are far more likely to be operating under the assumption that os.ErrPermission is a “do not retry this operation, but handle is still open for further use”, while SSH_FX_FAILURE is so generic that a client cannot really recover from the failure programmatically at all, as it is completely undefined and unknown whether any further operations are possible at that point.

Who is "we"?

This package, and the users of this package. I am specifically excluding any people who are forking this package. They are not “we”—the users of this package—because by using a fork, they are necessarily using a different package.

I think this principle is useful for deciding if one wants to invest the time into implementing something.. But in this case it is already implemented, and IMO is cleaner (for the reason described above).

Part of software development is deleting unnecessary code.

The original form of Server has used a map[string]*os.File since it was first introduced in a merged in September 2015. We have only now 9 years later, seen good cause to convert this into an interface. And this is despite already rolling out a better dynamic server API in RequestServer, because of which we have restricted the focus of Server to be strictly a very simple, thin, and transparent layer over the underlying OS filesystems. And the only reason we’re picking this up, is because Windows is a unique OS among all of the OSes that Golang supports.

When I tell you, “we’re not going to need that”, I mean it. It’s been almost a decade until we even needed the interface for a second implementation. There is no reason to prepare for a future that we’re unlikely to ever need. And “yeah, but I already did the work” does not negate, “and now we have to maintaining it.”

Indeed, in the very PR that introduced the map[string]*os.File an earlier version did implement this as an interface. But this was removed in favor of a simple map[string]*os.File presumably after realizing that Sever can simply use the concrete type, and so we didn’t need the interface. So, that work was undone, because it was unnecessary.

But don't expect me to change this while not giving any clear reasons, thanks 🙏

You are now, as you have been this whole time, under no obligation to do the work of implementing this PR. You volunteered for this task. However, in opposition to your ability to work on this at whatever pace you want all the way up to and including at any time “taking your ball and going home”, the same expectations are not on us as the contributors.

In order for your PR to be effected, I, or another contributor to this package, must sacrifice time out of our lives in order to engage with you, and polish the PR to the point, where we are comfortable with the code enough to merge it into this widely used and adopted package, and failure to respond promptly would be disrespectful to the time and effort you’ve put in. Even that means it comes at the expense of our weekend, or evenings. (Since I certainly don’t have the spare time during my work day to do this volunteer work.)

You must certainly understand that while you might commit this code, and then move on to other projects, the contributors of this repo have volunteered to stick around and support all of the code in this repo long-term. It is not unreasonable for us to push back on overly complex code implementing features that we are not using, and see no immediate utility to in the future.

If this dummyFile type is so useful that it becomes necessary, then we can refactor our code at that point. Until then, we have no use for this code.

But don't expect me to change…

And we are under no obligation to merge your code. I reasonably suggested to you that you should remove it, and even before this more detailed and extensive post, I had provided sufficient explanation as to why. And now almost 2 hours of research in order to pedantically track down references to provide context, and dive through the commit history of this package in order to dig up points to justify my argument, I’m unsure what you ever expected my behavior to be.

I am a volunteer that no matter what else I do stand in representation for the group of all contributors who are volunteering their time. Being directly rude, actively aggressive, or abusive would be wholly inappropriate, and reflect poorly on the whole package’s contributors. So, I must be patient, but insistent. If you interpret this professional courtesy as “passive aggressive”, then I’m sorry.

If you have any feedback I’m happy to listen to it. But unfortunately, as a person who will be responsible for maintaining this code after your PR is merged, I continue to insist that this change be made, whether you find any rational reason behind it or not.

Copy link
Author

@powellnorma powellnorma Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the thorough explanation.

Clients are far more likely to be operating under the assumption that os.ErrPermission is a “do not retry this operation, but handle is still open for further use”, while SSH_FX_FAILURE is so generic that a client cannot really recover from the failure programmatically at all, as it is completely undefined and unknown whether any further operations are possible at that point.

Okay, if that is the case I guess SSH_FX_PERMISSION_DENIED is practically the more appropriate choice.
However, I want to note, that it feels like SSH_FX_PERMISSION_DENIED then takes the role of "generic error" that SSH_FX_FAILURE perhaps should have had.
And I still think that os.ErrPermission is / can be misleading to the end-user who reads this error message.

An error like ENOSYS would be ideal I guess. Hmm, SSH_FX_OP_UNSUPPORTED comes to mind.

indicates that an attempt was made to perform an operation which is not supported for the server

[..] The server supports this operation. It just cannot perform this operation on that specific handle.

Well, one could say that the server does not implement e.g. copying a directory to / (the virtual root). But it could be implemented to e.g. mean "create a new drive and copy the folder contents onto there".

So I think it would be more similar to when e.g. Linux gives ENOSYS (or EOPNOTSUPP) for deleting a file for a specific filesystem. It's not that linux is incapable of deleting files, but just the current implementation of the filesystem is (thus rendering linux unable to do that for that filesystem).

The original form of Server has used a map[string]*os.File since it was first introduced in a merged in September 2015. We have only now 9 years later, seen good cause to convert this into an interface

If you think about it, this argument goes both ways. You originally said that implementing this would be a lot of work, and you don't see anyone having time for that. In effect, a useful feature thus might have never been implemented, because the code looks too "tedious" to be refactored/extended in such a way.

I see your point of keeping implementations "clean and lean". However, I'd say if it is not "a lot of complexity" that gets added, the advantages (easier to be extended in the future) outweighs the disadvantage.
And in this case, I personally even feel like separating dummyFile and winRoot is cleaner, even if one only uses dummyFile once.

But I guess it's a matter of taste, so I have changed the code according to your wishes.

So, I must be patient, but insistent. If you interpret this professional courtesy as “passive aggressive”, then I’m sorry.

No, what I perceived as "passive aggressive" was when you just repeated things you already said, which I had given an answer to, but you (thereby) ignored what I said.
It felt as if you were not willing to listen to the points I made, and instead just were half-annoyed that I dared to question some of your statements.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, SSH_FX_OP_UNSUPPORTED comes to mind.

I had already addressed the appropriateness of this status value 5 days ago: #569 (comment)

So I think it would be more similar to when e.g. Linux gives ENOSYS (or EOPNOTSUPP) for deleting a file for a specific filesystem.

Except that ENOSYS is not listed at all for unlink(2) https://man7.org/linux/man-pages/man2/unlink.2.html

In fact, instead, it is noted specifically that for linux:

       EPERM (Linux only)
              The filesystem does not allow unlinking of files.

EOPNOTSUPP is defined for sockets only, (https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03)

[EOPNOTSUPP]
Operation not supported on socket. The type of socket (address family or protocol) does not support the requested operation. A conforming implementation may assign the same values for [EOPNOTSUPP] and [ENOTSUP].

You might note that it allows for it to be conflated with ENOTSUP. However, even when conflated, only socket operations should be returning the EOPNOTSUPP synonym, while other operations should still be returning ENOTSUP.

As for the appropriateness of ENOTSUP not only does my argument against SSH_FX_OP_UNSUPPORTED still apply, so does my argument against ENOSYS.

because the code looks too "tedious" to be refactored/extended in such a way.

You’ve mistaken entirely what my anticipation of the tedium would be.

Your originally proposed method of implementing this was to modify every single point in the code where we called getHandle and treat a magic handle token specially. I warned you against that course of action, because this would have indeed been horrendously tedious. I directed you instead, to the least tedious implementation.

Instead, the tedium, which I anticipated, was never in constructing a pseudo-directory implementation. It was anticipating the process of ensuring everything worked properly, and that we covered all the “gotchas” that invariably result from implementing a parallel implementation—in this case to *os.File. Namely:

  • if you call Readdir(n) with an n > 0, you have to ensure to return a most that number of entries.
  • if you call Readdir(n) with an n <= 0, you have to ensure that it returns all entries.
  • when you call Readdir a second time, you should not return any of the entries already returned.
  • once Readdir is called and there are no remaining entries, it must return io.EOF, but not ever before
  • the Readdir has to return filenames that actually reflect the usage that clients should be using them as (i.e. c: not just c)
  • Checking for what os.Stat("c:\") actually returns, and how we need to change it in order to fit our use-case, so that we can be sure we’re returning the proper and appropriate values from all of the fs.FileInfo values for the individual drives
  • Properly encoding the toLocalPath operations to ensure that we don’t allow accesses to \Windows but instead require a drive name

No, what I perceived as "passive aggressive" was when you just repeated things you already said, which I had given an answer to, but you (thereby) ignored what I said.
It felt as if you were not willing to listen to the points I made, and instead just were half-annoyed that I dared to question some of your statements.

If I have repeated myself, it is because you refused to acknowledge or listen to the point I had already made, or brought up a point we had already moved from, while trying to get me to relitigate it. (see, above, where you again drag out SSH_FX_OP_UNSUPPORTED after I had already written a detailed essay about why it would be inappropriate.)

Your arguments have repeatedly refused to actually listen to anything for which I was taking hours to research and compose, in order to patiently explain all this to you.

Even now, your response is not, “I understand. Thank you for taking so much time explaining these fine, minute, and subtle details, and helping to ensure that my code covers all of the gotcha corner cases involved in the standards. Let me know if this code addresses your concerns,” but instead you have answered, “I still refuse to acknowledge your arguments. I think I’m still right, but I have constructed a face-saving reason for me to comply. So, I will do it your way.”

You have mistaken “no matter how many times you repeat this argument, the standards and references continue to refute it,” for “I am ignoring your arguments.”

If I were ignoring your arguments, I would not be pulling up the POSIX documentation in order to evaluate the validity and soundness of your arguments. Meanwhile, in this single message alone, you’ve:

  • repeated an already refuted SSH_FX_OP_UNSUPPORTED
  • thrown out ENOSYS in a situation where under your very own scenario, linux is returning EPERM
  • and tossed in EOPNOTSUPP without actually reading the POSIX standard to see that it is entirely constrained to sockets.

To me, it really does seems like you are not only not reading my explanations; you’re not even reading your own references.


Under advisement from @drakkan , this matter has gotten quite a bit out of hand, so we’re parking everything involved for 2 weeks, so that all parties (including me) can cool off, and we can come back to this later, when emotions are not being run raw.

Since I am both: the person with an imbalance of control in this situation, and also the one closing down the discussion; I want to be clear that I’m not doing this to silence you. You may post a reasonable response, but no further actions will be taken from my side for two weeks.†

†: With the caveat that there is one linting comment to address a typo, which will follow this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like if we look at the bigger picture, we both mostly have the same goal of making this software more useful. It's not useful for either of us to spend much time discussing minor details, like whether to implement something in one way or a slightly different way.

EOPNOTSUPP is defined for sockets only

Two things:

sshfs itself uses this error code when it gets an 'SSH_FX_OP_UNSUPPORTED':
https://github.com/libfuse/sshfs/blob/sshfs-3.7.3/sshfs.c#L1704

If we look at the linux source code, we see that it is actually used a lot in filesystem (fs/*) code:
https://elixir.bootlin.com/linux/v4.0/ident/EOPNOTSUPP

As for the appropriateness of ENOTSUP not only does my argument against SSH_FX_OP_UNSUPPORTED still apply

Your argument was that the server does generally support the operation (e.g. unlink), so it should not return this error.
However, my counter-argument was that "operation" might entail the subject (e.g. the path) that is operated on.

If we look at the docs for SSH_FX_PERMISSION_DENIED, they say: The user does not have sufficient permissions to perform the operation.
So it seems that here the term "operation" clearly entails the subject (path) that is operated on, because the user probably can e.g. unlink his own files.

I don't think you have addressed that "counter-argument" yet, but instead you say things like repeated an already refuted SSH_FX_OP_UNSUPPORTED.

Maybe you have just missed/overlooked that, it's not (overall) a big deal. I think it will help us to assume that the other person had good intentions. Then probably it will be easier for us to see the point the other person is making, instead of going into some kind of "defensive mode".

Instead, the tedium, which I anticipated, was never in constructing a pseudo-directory implementation

You said: just even adding support to use a pseudo-directory in Server would be a pain, because right now our handles are all mapping directly to an actual *os.File which—as noted above—cannot exist for this pseudo-directory.

This clearly references the restrictiveness of the old implementation, and that (alone) would be "a pain".

And true, making sure that everything is implemented correctly is quite some work too! And I am grateful that you took the time to review it that carefully.

but instead you have answered “I still refuse to acknowledge your arguments. I think I’m still right, but I have constructed a face-saving reason for me to comply. So, I will do it your way.”

I said:

  • Thank you for the thorough explanation.
  • Okay, if that is the case I guess SSH_FX_PERMISSION_DENIED is practically the more appropriate choice
  • I see your point of keeping implementations "clean and lean"

Still, I have my own opinions. And while I might do X as requested because I agree with you on Y, it doesn't mean that this has to "invalidate" my previous opinion Z which may also be about X.

To me, it really does seems like you are not only not reading my explanations; you’re not even reading your own references.

I don't think such accusations are helpful in any way.

Besides, I am not even sure what you mean by "own references". I mostly just said that I think for the enduser a specific error code could be confusing. And that another one seems more fitting IMO.

I don't think that there is a clear "right" or "wrong", which is probably why one could argue about this endlessly. But I don't think there is much value in that discussion, as both "perspectives" are somewhat valid, and in the end its IMO a minor detail of this feature.


// Server is an SSH File Transfer Protocol (sftp) server.
// This is intended to provide the sftp subsystem to an ssh server daemon.
// This implementation currently supports most of sftp server protocol version 3,
Expand All @@ -30,13 +76,13 @@ type Server struct {
debugStream io.Writer
readOnly bool
pktMgr *packetManager
openFiles map[string]*os.File
openFiles map[string]file
openFilesLock sync.RWMutex
handleCount int
workDir string
}

func (svr *Server) nextHandle(f *os.File) string {
func (svr *Server) nextHandle(f file) string {
svr.openFilesLock.Lock()
defer svr.openFilesLock.Unlock()
svr.handleCount++
Expand All @@ -56,7 +102,7 @@ func (svr *Server) closeHandle(handle string) error {
return EBADF
}

func (svr *Server) getHandle(handle string) (*os.File, bool) {
func (svr *Server) getHandle(handle string) (file, bool) {
svr.openFilesLock.RLock()
defer svr.openFilesLock.RUnlock()
f, ok := svr.openFiles[handle]
Expand Down Expand Up @@ -85,7 +131,7 @@ func NewServer(rwc io.ReadWriteCloser, options ...ServerOption) (*Server, error)
serverConn: svrConn,
debugStream: ioutil.Discard,
pktMgr: newPktMgr(svrConn),
openFiles: make(map[string]*os.File),
openFiles: make(map[string]file),
}

for _, o := range options {
Expand Down Expand Up @@ -462,7 +508,7 @@ func (p *sshFxpOpenPacket) respond(svr *Server) responsePacket {
osFlags |= os.O_EXCL
}

f, err := os.OpenFile(svr.toLocalPath(p.Path), osFlags, 0o644)
f, err := openfile(svr.toLocalPath(p.Path), osFlags, 0o644)
if err != nil {
return statusFromError(p.ID, err)
}
Expand Down
13 changes: 13 additions & 0 deletions server_posix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//go:build !windows
// +build !windows

package sftp

import (
"io/fs"
"os"
)

func openfile(path string, flag int, mode fs.FileMode) (file, error) {
return os.OpenFile(path, flag, mode)
}
89 changes: 89 additions & 0 deletions server_windows.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
package sftp

import (
"fmt"
"io"
"io/fs"
"os"
"path"
"path/filepath"

"golang.org/x/sys/windows"
)

func (s *Server) toLocalPath(p string) string {
Expand Down Expand Up @@ -37,3 +43,86 @@ func (s *Server) toLocalPath(p string) string {

return lp
}

func bitsToDrives(bitmap uint32) []string {
var drive rune = 'a'
var drives []string

for bitmap != 0 {
if bitmap&1 == 1 {
drives = append(drives, string(drive)+":")
}
drive++
bitmap >>= 1
}

return drives
}

func getDrives() ([]string, error) {
mask, err := windows.GetLogicalDrives()
if err != nil {
return nil, fmt.Errorf("GetLogicalDrives: %w", err)
}
return bitsToDrives(mask), nil
}

type driveInfo struct {
fs.FileInfo
name string
}

func (i *driveInfo) Name() string {
return i.name // since the Name() returned from a os.Stat("C:\\") is "\\"
}

type winRoot struct {
dummyFile
drives []string
}

func newWinRoot() (*winRoot, error) {
drives, err := getDrives()
if err != nil {
return nil, err
}
return &winRoot{
drives: drives,
}, nil
}

func (f *winRoot) Readdir(n int) ([]os.FileInfo, error) {
drives := f.drives
if n > 0 {
if len(drives) > n {
drives = drives[:n]
}
f.drives = f.drives[len(drives):]
if len(drives) == 0 {
return nil, io.EOF
}
powellnorma marked this conversation as resolved.
Show resolved Hide resolved
}

var infos []os.FileInfo
for _, drive := range drives {
fi, err := os.Stat(drive)
if err != nil {
return nil, err
}
powellnorma marked this conversation as resolved.
Show resolved Hide resolved

di := &driveInfo{
FileInfo: fi,
name: drive,
}
infos = append(infos, di)
}

return infos, nil
}

func openfile(path string, flag int, mode fs.FileMode) (file, error) {
if path == "/" {
powellnorma marked this conversation as resolved.
Show resolved Hide resolved
return newWinRoot()
}
return os.OpenFile(path, flag, mode)
}