In Windows, Server should allow a pseudo-directory for / which enumerates available drives

[powellnorma]

When I use sshfs host:/ mnt, the current working directory gets mounted to mnt.
Is it possible to let the sshfs server behave the way a cygwin compiled openssh server does, e.g. that it lists /c:, /d:, ... in the root directory?

[puellanivis]

This is not the issue board for the sshfs project? I don’t believe anyone here has knowledge of how this would be done, and it wouldn’t really be an appropriate place for it even if someone did.

You can checkout their project’s issue board her: https://github.com/libfuse/sshfs/issues?q=is%3Aissue

[powellnorma]

Isn't it the sftp server that answers with what the content of / is? Currently for some reason it just gives me the entries of the current working directory the sftp server was started in.

[puellanivis]

The sftp server does indeed answer what the contents of / are. If you’re running some sort of ssh server based on this library, and it is showing you the entries of the current working directory, then this is a matter for either: 1) your individual implementation of RequestServer or A) sshfs as we just use whatever path the client sends, and if it’s sending relative paths, then we’re going to share relative paths.

Without more details about how you’re actually using this package, we really cannot answer anything.

[powellnorma]

I use the example from here:
https://github.com/gliderlabs/ssh/blob/master/_examples/ssh-sftpserver/sftp.go

It seems to use the default RequestServer Handler? Do you have an idea why it behaves the way I described for /?

[puellanivis]

Hm, that code is not using RequestServer but instead is using Server. This is a very minimal wrapper around system calls, and limited in how useful it is.

After some trouble, I’ve managed to set up a repro. My behavior is that I get a listing of C:\ when I use cd / ; dir in sftp. This is the same as if I cd /c:/ ; dir. However, if I cd /d:/ ; dir I get a listing of that drive instead.

Getting this working is going to be a piece of work. This definitely isn’t anything easy, because there’s no actual directory to stat or readdir from to get an enumeration of available drives. So we would need to write a whole separate pathway to support one single pseudo-directory for only one OS, which handles the / case, and enumerates the windows drives… 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. And the Server implementation already has known limitations that we’re unwilling to address, because we would like people to switch to RequestServer as a more generic interface, rather than being a raw passthrough into the underlying filesystem.

😩 So, I’m sorry to give sad news, but I don’t think we’ll actually be able to get this feature done. If you want to sit down and invest the work necessary to get it done, we would consider a PR for this, as it’s not an unreasonable feature… just a lot of work, which I don’t think any of volunteers have time to help with.

[powellnorma]

I think it would make sense to put here, in handlePacket:

sftp/server.go

Lines 177 to 180 in 22452ea

	func handlePacket(s *Server, p orderedRequest) error {
	var rpkt responsePacket
	orderID := p.orderID()
	switch p := p.requestPacket.(type) {

a call to a new handleSepcialPath function, that is implemented per OS. In server_windows.go we then check if p.Path == "/" and if so handle it in a special way. For getting a list of logical drives we could use https://stackoverflow.com/a/23135463

What do you think about that approach?

And the Server implementation already has known limitations that we’re unwilling to address, because we would like people to switch to RequestServer as a more generic interface, rather than being a raw passthrough into the underlying filesystem

How would I implement it with that interface? Do I need to implement all the Handlers Interfaces myself? That seems like a lot of work, or am I missing something?

[puellanivis]

Again, one doesn’t just list a directory, first you SSH_FXP_OPENDIR and then you issue SSH_FXP_READDIR packets to read entries of that directory. This means we must work it into the handle system, which as noted, currently is a map[string]*os.File this will require reworking the whole of the handle system to allow an interface that incorporates all methods we use from os.File in order to also parallel implement the pseudo directory.

Do not use the code in that stackoverflow, use https://pkg.go.dev/golang.org/x/sys/windows#GetLogicalDrives also OMG, do not use the bitsToDrives given use:

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
}

As for how you would implement it in the RequestServer I’m going to admit that I am less knowledgeable in that area. I’m not familiar with it, and how it would be implemented, but it should likely but significantly easier, but of course, with more code, since the interface is more flexible than just using the raw underlying filesystem.

[drakkan]

Hello and Happy New Year

How would I implement it with that interface? Do I need to implement all the Handlers Interfaces myself? That seems like a lot of work, or am I missing something?

Yes, you have to implement at least these 4 handlers/methods yourself and any other optional interface needed for your use case.

This may seem like a lot of work initially, but it allows you to make the SFTP server work the way you want and have much more control than using the old Server implementation.

[powellnorma]

Happy new year!

Again, one doesn’t just list a directory, first you SSH_FXP_OPENDIR and then you issue SSH_FXP_READDIR packets to read entries of that directory. This means we must work it into the handle system

I see handle is a string, which normally always just encodes a number. Why not make it such that in case of / on windows, instead of normal nextHandle the method sshFxpOpenPacket.respond stores the pair ("root-%d", nil) ? Then in sshFxpReaddirPacket.respond we check if strings.HasPrefix(p.Handle, "root-"), and if true handle it differently.

And for making sure that other methods don't get confused by the nil handle (in case the user for some reason requests other operations than "readdir" for that handle), we could adjust Server.getHandle to check if f is nil, and if so just act as if doesn't exist.

What do you think of that approach?

This may seem like a lot of work initially, but it allows you to make the SFTP server work the way you want and have much more control than using the old Server implementation.

Is there some example of a standard SFTP server I could use as a basis? Like Server but implemented via the Handlers Interface?

[drakkan]

Is there some example of a standard SFTP server I could use as a basis? Like Server but implemented via the Handlers Interface?

The example in the repo uses InMemHandler. Several projects uses the request server, for example SFTPGo and rclone. Take a look here for example

[drakkan]

Here is a simple example to use as base (review carefully the code before using it in production)

package main

import (
	"fmt"
	"io"
	"log"
	"net"
	"os"

	"github.com/pkg/sftp"
	"golang.org/x/crypto/ssh"
)

func main() {
	hostKey := []byte(`-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
QyNTUxOQAAACAY/pePhhw4I5c/PP0RDZgNhbqggxvnlkObssNIDqJWLAAAAJBVB0cQVQdH
EAAAAAtzc2gtZWQyNTUxOQAAACAY/pePhhw4I5c/PP0RDZgNhbqggxvnlkObssNIDqJWLA
AAAEB+jbwnXNd0sypQhBp1TiJVb2y74QH/0ywKkh7B7/UxEhj+l4+GHDgjlz88/RENmA2F
uqCDG+eWQ5uyw0gOolYsAAAACW5pY29sYUBwMQECAwQ=
-----END OPENSSH PRIVATE KEY-----`)

	private, err := ssh.ParsePrivateKey(hostKey)
	if err != nil {
		log.Fatal(err)
	}
	config := &ssh.ServerConfig{
		PasswordCallback: func(c ssh.ConnMetadata, pass []byte) (*ssh.Permissions, error) {
			if c.User() == "testuser" && string(pass) == "tiger" {
				return nil, nil
			}
			return nil, fmt.Errorf("password rejected for %q", c.User())
		},
	}
	config.AddHostKey(private)

	listener, err := net.Listen("tcp", "0.0.0.0:20222")
	if err != nil {
		log.Fatal("failed to listen for connection", err)
	}
	fmt.Printf("Listening on %v\n", listener.Addr())

	nConn, err := listener.Accept()
	if err != nil {
		log.Fatal("failed to accept incoming connection", err)
	}

	// Before use, a handshake must be performed on the incoming net.Conn.
	sconn, chans, reqs, err := ssh.NewServerConn(nConn, config)
	if err != nil {
		log.Fatal("failed to handshake", err)
	}
	log.Println("login detected:", sconn.User())

	go ssh.DiscardRequests(reqs)

	for newChannel := range chans {
		// Channels have a type, depending on the application level
		// protocol intended. In the case of an SFTP session, this is "subsystem"
		// with a payload string of "<length=4>sftp"
		if newChannel.ChannelType() != "session" {
			newChannel.Reject(ssh.UnknownChannelType, "unknown channel type")
			continue
		}
		channel, requests, err := newChannel.Accept()
		if err != nil {
			log.Fatal("could not accept channel.", err)
		}

		// Sessions have out-of-band requests such as "shell",
		// "pty-req" and "env".  Here we handle only the
		// "subsystem" request.
		go func(in <-chan *ssh.Request) {
			for req := range in {
				ok := false
				switch req.Type {
				case "subsystem":
					if string(req.Payload[4:]) == "sftp" {
						ok = true
					}
				}
				req.Reply(ok, nil)
			}
		}(requests)

		h := &rsHandler{}
		server := sftp.NewRequestServer(channel, sftp.Handlers{
			FileGet:  h,
			FilePut:  h,
			FileCmd:  h,
			FileList: h,
		})
		if err := server.Serve(); err != nil {
			if err != io.EOF {
				log.Fatal("sftp server completed with error:", err)
			}
		}
		server.Close()
		log.Print("sftp client exited session.")
	}

}

type rsHandler struct{}

func (h *rsHandler) Fileread(r *sftp.Request) (io.ReaderAt, error) {
	f, err := os.Open(r.Filepath)
	if err != nil {
		return nil, err
	}
	return f, nil
}

func (h *rsHandler) Filewrite(r *sftp.Request) (io.WriterAt, error) {
	f, err := os.OpenFile(r.Filepath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0777)
	if err != nil {
		return nil, err
	}
	return f, nil
}

func (h *rsHandler) Filecmd(r *sftp.Request) error {
	switch r.Method {
	case "Rename":
		return os.Rename(r.Filepath, r.Target)
	case "Rmdir", "Remove":
		return os.Remove(r.Filepath)
	case "Mkdir":
		return os.Mkdir(r.Filepath, os.ModePerm)
	default:
		return sftp.ErrSSHFxOpUnsupported
	}
}

func (h *rsHandler) Filelist(r *sftp.Request) (l sftp.ListerAt, err error) {
	switch r.Method {
	case "List":
		f, err := os.Open(r.Filepath)
		if err != nil {
			return nil, err
		}
		files, err := f.Readdir(-1)
		f.Close()
		if err != nil {
			return nil, err
		}
		return listerAt(files), nil
	case "Stat":
		fi, err := os.Stat(r.Filepath)
		if err != nil {
			return nil, err
		}
		return listerAt([]os.FileInfo{fi}), nil
	default:
		return nil, sftp.ErrSSHFxOpUnsupported
	}
}

type listerAt []os.FileInfo

func (l listerAt) ListAt(f []os.FileInfo, offset int64) (int, error) {
	if offset >= int64(len(l)) {
		return 0, io.EOF
	}

	n := copy(f, l[offset:])
	if n < len(f) {
		return n, io.EOF
	}
	return n, nil
}

[puellanivis]

Again, one doesn’t just list a directory, first you SSH_FXP_OPENDIR and then you issue SSH_FXP_READDIR packets to read entries of that directory. This means we must work it into the handle system

I see handle is a string, which normally always just encodes a number. Why not make it such that in case of / on windows, instead of normal nextHandle the method sshFxpOpenPacket.respond stores the pair ("root-%d", nil) ? Then in sshFxpReaddirPacket.respond we check if strings.HasPrefix(p.Handle, "root-"), and if true handle it differently.

And for making sure that other methods don't get confused by the nil handle (in case the user for some reason requests other operations than "readdir" for that handle), we could adjust Server.getHandle to check if f is nil, and if so just act as if doesn't exist.

What do you think of that approach?

I mean, we could do something like this, but we would still need to implement a second pseudo *os.File. Basically, this wouldn’t actually save any effort over doing it the way I already said. It’s not hard work what I mentioned, it’s just a lot of work. And we’re all volunteers here, finding spare time during our day to provide support.

[powellnorma]

I mean, we could do something like this, but we would still need to implement a second pseudo *os.File

I might be missing something obvious, but why is that? In my understanding, all we need for the "virtual root" is to replace the "filling of sshFxpNamePacket.NameAttrs via f.Readdir" with a "fill sshFxpNamePacket.NameAttrs via a 'list logical drive letters' function"?

We can't copy into that folder anyways etc - So I think it might be fine to let Server.getHandle give an "handle does not exist" error. Are there any other legitimate file operations for this virtual directory, other than readdir?

And we’re all volunteers here, finding spare time during our day to provide support.

You are right, and it is appreciated! 🙂 My goal is to figure out how much work it would be to realize this, and why. I have the hope that it is not as much work as one might first think 🤞

[puellanivis]

Because that would be very surprising behavior to the client. The client would opendir("/") then fsetstat(mode: 0o755) and be confronted with a handle does not exist error? That’s a pretty surprising error for a handle that you definitely know the server just gave you, so it better exist. And each and every getHandle call point would need to be modified to handle getHandle now returning an error instead of a bool, or every getHandle call point would have to be changed to handle the case of a "root-%d" handle even when ok == true and return an appropriate error, rather than—as mentioned—an inappropirate handle does not exist for a handle that totally does exist, but just won’t permit the action. And we’ll still need to properly implement Close() behavior on that handle as well, which is another can of special casing worms.

Now, instead of changing each call point, we can leverage the duck typing and type inference of Golang, and have getHandle return instead some type fileHandle interface{ … } which implements the subset of all the functions from *os.File that we use, which is either an actual *os.File, or a type driveEnumerationDir that implements—for example—Chmod with something like “permission denied”. Each of these stubs is 3–4 lines of code of just the function signature and a return nil, appropriateError, so this should be relatively easy. And now, we don’t have to change every call site, because we’ve leveraged the design principles of Golang to make the refactor easy. Then we just have to modify the opendir code to special case "/" on Windows.

[powellnorma]

That’s a pretty surprising error for a handle that you definitely know the server just gave you, so it better exist

I see. An "permission denied" could also be suprising though, if one starts the server as an windows admin. In the end the user of sftp has to handle '/' specially on windows either way (in such cases where he previously assumed he can do fsetstat etc).

Perhaps the "handle does not exist" even makes sense to a degree.. because that path really does not exist on windows?

Currently we have the situation in which '/' does not contain all drives. If one would want to e.g. create a full copy of the entire system via sftp, a script that does "scp user@host:/ /path/to/backup" would currently not work as expected.

And each and every getHandle call point would need to be modified to handle getHandle now returning an error instead of a bool

Yes, but not if we'd be ok with giving the client a "handle not found" error - I see that doing an "open" and then afterwards getting (on an subsequent operation) an "does not exist" is weird. But perhaps it would be an acceptable corner/edge case?

Now, instead of changing each call point, we can leverage the duck typing and type inference of Golang, and have getHandle return instead some type fileHandle interface{ … } which implements the subset of all the functions from *os.File that we use, which is either an actual *os.File, or a type driveEnumerationDir that implements—for example—Chmod with something like “permission denied”. Each of these stubs is 3–4 lines of code of just the function signature and a return nil, appropriateError, so this should be relatively easy

Ok, that sounds like a great solution!
Do you think "permission denied" would be the appropriate for all file operations?

[puellanivis]

You are wrong in your assessment that “permission denied” is surprising. It is the normal expected error from requesting performance of an operation that cannot be performed. It says that the given operation should not be reattempted, while also being silent as to whether any other operations may still otherwise be possible. I would be equally unsurprised if attempting to FSETSTAT would fail on a handle for the directory /c:/.

“Handle does not exist” would need to means that the sftp server does not have the handle that it just created internally, which it gave out to the client for use. It is an sftp-specific term, which is separate from the term as used by the OS. In fact, for the InMemoryHandler all of its handles are fictitious with respect to the OS, because all of them exist internally to the application in memory. Such an error also means that any client receiving that error should presume that the handle does not exist, and not attempt any action on that handle, even an SSH_FXP_CLOSE. The correct response to us returning an “handle does not exist” error would be to immediately throw away that handle, and forget the client ever thought it existed.

Currently we have the situation in which '/' does not contain all drives. If one would want to e.g. create a full copy of the entire system via sftp, a script that does "scp user@host:/ /path/to/backup" would currently not work as expected.

You are not wrong here. However, the SFTP standard we use does not define anything about how this should work in Windows. I have already acknowledged that you have made a reasonable request, because interoperability is important. However, our code is current standards compliant in this area.

Note that DIR \ will print the top-level of the current Windows drive. It is not unreasonable that our service translates a path of / to \ , and point it to the top-level of the current Windows drive. This is a well-known Unix/Windows interoperability friction point. Last I looked, Cygwin itself uses the notation /cygdrive/d rather than /d:/ in this particular situation.

But perhaps it would be an acceptable corner/edge case?

I will not accept a "handle not found" error in this case, as I have already explained it is misleading and confusing, and directly and unambiguously points to the problem being in a completely different area of our systems. Particularly when, as mentioned, “permission denied” exists and is the perfectly normal, “yeah, you can’t do that on this file/directory” message.

Do you think "permission denied" would be the appropriate for all file operations?

I do not know. I have not evaluated all of the operations that would need to be implemented.

[powellnorma]

You are wrong in your assessment that “permission denied” is surprising. It is the normal expected error from requesting performance of an operation that cannot be performed.

I agree that returning "no such file" is a bit misleading in this case.
Hm, I just looked at https://winscp.net/eng/docs/sftp_codes - Maybe Operation unsupported (or Invalid parameter) would fit even better?

Interesting, there is also a difference between Invalid handle, No such file, No such path. I guess on my first approach (just acting as if the handle does not exist) we would have returned Invalid handle?

I have opened #571 - it implements your suggestion of replacing *os.File with a more general FileLike interface

[puellanivis]

Hm, I just looked at https://winscp.net/eng/docs/sftp_codes - Maybe Operation unsupported (or Invalid parameter) would fit even better?

No. It is not appropriate.

   SSH_FX_OP_UNSUPPORTED
      indicates that an attempt was made to perform an operation which
      is not supported for the server (it may be generated locally by
      the client if e.g.  the version number exchange indicates that a
      required feature is not supported by the server, or it may be
      returned by the server if the server does not implement an
      operation).

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

Invalid parameter was adopted at with https://filezilla-project.org/specs/draft-ietf-secsh-filexfer-07.txt#section-8 not with filexfer-02, which is the version we’re targeting. But as well, none of the parameters to the supposed SSH_FXP_FSETSTAT would actually be invalid anyways. The operation itself cannot be performed.

Interesting, there is also a difference between Invalid handle, No such file, No such path.

This is not unusual at all. As stated:

• invalid handle means the client sent a handle that the server does not recognize. (Therefore, only on an operation with a handle as a parameter.
• no such file is returned when a file does not exist. (Therefore, never on an operation with a handle as a parameter. But this may or may not have been caused by the containing path existing.) This was the only error available to be returned with filxfer-02.
• no such path is returned if the file does not exist, or is invalid. This was introduced with filexfer-03 (This can also never happen on an operation with a handle as a parameter. But the standard has now been expanded to add a specific error to indicate that the containing path does not exist, so simply adding a SSH_FXF_CREAT to your pflags in the open won’t help.

The only real alternative to SSH_FX_PERMISSION_DENIED is SSH_FX_FAILURE with a message This operations cannot be performed on this handle. But 🤷‍♀️ that’s effectively just permission denied by other words.

[powellnorma]

This operations cannot be performed on this handle. But 🤷‍♀️ that’s effectively just permission denied by other words.

I personally think This operations cannot be performed on this handle seems more fitting, as its not a problem with permissions, but just that copying to the virtual / on windows is not implemented - If you don't disagree, I'd adjust it accordingly.

Instead of os.ErrPermission what can we return such that statusFromError maps it to sshFxFailure? I see that sshFxFailure seems to be the default error code - So maybe we could just define a new error type and not handle it in statusFromError?

[puellanivis]

Yes, I still disagree. Please read more carefully in the future:

But 🤷‍♀️ that’s effectively just permission denied by other words.

[powellnorma]

Why so passive aggressive? For me these seem to be different things..

• Permission denied: The user could potentially fix this by redoing the process with a different user, that has higher privileges
• Operations cannot be performed on this handle: The handle itself is not suitable for this operation, it probably is in the nature of the resource the handle refers to, that this cannot be performed

I feel the latter is more matching, and that there is a clear difference. If you insist I am fine with "permission denied" too, ofc

[cmprmsd]

@powellnorma
Props for the PR!
I also had the same issue and tried solving the whole problem with the interface handlers. As I got stuck and it did not work in the end I ended up looking here and seeing you having the same issue within some weeks got me laughing 😄

After some trouble, I’ve managed to set up a repro. My behavior is that I get a listing of C:\ when I use cd / ; dir in sftp. This is the same as if I cd /c:/ ; dir. However, if I cd /d:/ ; dir I get a listing of that drive instead.

@puellanivis
This is a great workaround for the mean time! Thank you. 😊
For sshfs this would be:
sshfs -p 2222 'user@<ip>:/z:' mountpoint