server.go: "/" for windows

[powellnorma]

fix #569

[puellanivis]

I suppose I want to add that changing from this / is the root of the current drive, to / is the drive-space root is a breaking change of behavior, and should properly be gated behind a https://pkg.go.dev/github.com/pkg/sftp#ServerOption

[powellnorma]

I suppose I want to add that changing from this / is the root of the current drive, to / is the drive-space root is a breaking change of behavior, and should properly be gated behind a https://pkg.go.dev/github.com/pkg/sftp#ServerOption

Ok, but I'd think it would be good to make it opt-out by default - Because I'd guess most people have not yet thought about this edge case, and would not expect the old behavior. What do you think?

[puellanivis]

The correct default state of the option should be the old behavior. Because it’s the current behavior, and we don’t know if people are depending on this current behavior.

[powellnorma]

How to add the an additional server option? Can you please help out?

Edit: Ok fixed in latest commit

[drakkan]

@powellnorma I'm not familiar with the old Server implementation and so I can't help you much here and I don't even have time to do so.

The feature you propose makes sense and would be useful for Windows users (although you can use RequestServer as an alternative) but please:

1. remember we are volunteers, no one pays us for this work or to help you so don't request for step by step help and carefully test your code before sending a PR
2. respect the guideline of the project owners, @puellanivis is a really experienced developer, she is very thorough in reviewing code and test cases and I really appreciated when she helped me improve my PRs in the past

Thanks for understanding and for trying to contribute to this package

[drakkan]

Not sure if I'm testing this PR in the correct way but I get this

sftp> ls /
Can't ls: "/" not found

I'm testing using examples/go-sftp-server/main.go with this change (it would be useful to add an option for win root to the example)

git diff examples/go-sftp-server/main.go
diff --git a/examples/go-sftp-server/main.go b/examples/go-sftp-server/main.go
index ba902b6..30d1071 100644
--- a/examples/go-sftp-server/main.go
+++ b/examples/go-sftp-server/main.go
@@ -127,6 +127,7 @@ func main() {
                } else {
                        fmt.Fprintf(debugStream, "Read write server\n")
                }
+               serverOptions = append(serverOptions, sftp.WindowsRootEnumeratesDrives())
 
                server, err := sftp.NewServer(
                        channel,

The sftp cli sends a lstat packet and the server errors out here

[puellanivis]

Oh wow, yeah, that’s not translating the remote path into local path. 😮

P.S. wait, no, it is calling into localpath (don’t do code-reviews buzzed folks). I think the problem here is that we are are calling Lstat on the file, which is not allowed. We would need to work around this to support it just like we did with OpenFile.

[puellanivis]

OK, I think we don’t want to block the existing changes on implementing Lstat on the \\?\ path.

But right now, until we have full compatibility with the openssh CLI client, we just cannot merge this. Or, we merge this, and then fix the issues ourselves… there are a few nitpicks I’d like to address as well anyways. Either way, I’d like to release a patch version covering the zos changes first.