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

Add support for working directory in Server #528

Merged
merged 12 commits into from
Oct 18, 2022

Conversation

mafredri
Copy link
Contributor

This commit allows the working directory for the (old) Server
implementation to be changed without doing a os.Chdir first.

The feature can be enabled with sftp.WithServerWorkingDirectory(dir)
passed as an option to sftp.NewServer.

It is useful when the sftp is used as part of a larger service that
does more than just serve sftp and using os.Chdir is not an option.

The fallback behavior (when the option is not specified) is that the
path remains unmodified (as before).

Motivation: I know the new sftp.RequestServer already supports this, however, since it currently only has an in-memory filesystem and is potentially going to see a larger refactor in the future, I think it's a good idea to include it for this older implementation as well.

mafredri added a commit to coder/coder that referenced this pull request Oct 14, 2022
This commit switches to our fork of `pkg/sftp` which includes a Server
option for changing the current working directory.

Attempt to upstream: pkg/sftp#528

Supercedes and closes #4420

Fixes #3620
mafredri added a commit to coder/coder that referenced this pull request Oct 14, 2022
This commit switches to our fork of `pkg/sftp` which includes a Server
option for changing the current working directory.

Attempt to upstream: pkg/sftp#528

Supercedes and closes #4420

Fixes #3620
@mafredri mafredri force-pushed the mafredri/add-workdir-to-server branch from da6a69d to 68783f7 Compare October 14, 2022 11:40
This commit allows the working directory for the (old) Server
implementation to be changed without doing a `os.Chdir` first.

The feature can be enabled with `sftp.WithServerWorkingDirectory(dir)`
passed as an option to `sftp.NewServer`.

It is useful when the `sftp` is used as part of a larger service that
does more than just serve `sftp` and using `os.Chdir` is not an option.

The fallback behavior (when the option is not specified) is that the
path remains unmodified (as before).
@mafredri mafredri force-pushed the mafredri/add-workdir-to-server branch from 68783f7 to 6a7168c Compare October 14, 2022 12:55
@drakkan
Copy link
Collaborator

drakkan commented Oct 16, 2022

Hi,

thanks for your PR. At first glance it looks ok, but I have never user the server implementation myself. If no other contributors have time to review this PR you will have to wait a while for a proper review. Thanks for your patience.

Please note that no larger refactor is planned for the request server, we just don't provide a sample implementation that uses the local filesystem but you can easily write your own implementation and it is much more customizable than the old server implementation. I'm not sure we'll continue to maintain the old server implementation in the future, maybe we'll rewrite it using the request server interfaces (and probably outside the root package)

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

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

As drakkan says, you should be able to just implement a raw OS RequestServer and I just don’t see the value in adding features to a deprecated implementation.

There are a large number of caveats in path handling, and making sure they’re all correct, compatible, and won’t break others is way harder than one might think.

request-plan9.go Outdated Show resolved Hide resolved
request-unix.go Outdated Show resolved Hide resolved
request-unix.go Outdated Show resolved Hide resolved
request_test.go Outdated Show resolved Hide resolved
request_test.go Outdated Show resolved Hide resolved
request_test.go Outdated Show resolved Hide resolved
request_windows.go Outdated Show resolved Hide resolved
request_test.go Outdated Show resolved Hide resolved
@mafredri
Copy link
Contributor Author

mafredri commented Oct 17, 2022

Did I misinterpret what was being discussed in #407? To me it sounds like a refactor/rework of the API, and I think it sounds wonderful TBH. But it's also a reason why I wasn't looking to work with the RequestServer at this time.

The fact that RequestServer is missing a FS implementation is also a reason I opted out, it means Server is what people reach for when they need SFTP, so it'd be more battle tested. (My reasoning may of course be wrong.)

As drakkan says, you should be able to just implement a raw OS RequestServer and I just don’t see the value in adding features to a deprecated implementation.

I appreciate the review, but I'm left wondering if I should clean up this PR or discard it? It sounds like you're not interested in merging it but I could be wrong.

I tried to make sure that no code paths are altered unless this new feature is enabled so that it'd be easier to merge in, but I can understand if you don't want it at all.

Edit: Made some updates anyway, happy to work on this further to get it merged, so let me know what you think/want.

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

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

It is not that I am not interested in merging this, but rather by making changes to a deprecated component, you are facing an uphill battle between “would rather not touch this” and “is this a valuable change.”

@puellanivis
Copy link
Collaborator

😐 It seems like github had been spamming out all of my draft responses. Learn from me, don’t do code review responses after a long hard day of toiling in the salt mines of legacy code…

server.go Outdated
@@ -256,13 +267,14 @@ func handlePacket(s *Server, p orderedRequest) error {
rpkt = statusFromError(p.ID, err)
}
case *sshFxpOpendirPacket:
p.Path = toLocalPath(p.Path)
lp := toLocalPath(s.workDir, p.Path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There’s another p.Path that wasn’t replaced by lp in the &sshFxpOpenPacket packet. I’d probably recommend backing out the s/lp/p.Path/ change instead.

Less change, less chance of new bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow. The only other assignment to p.Path that I can find is in (*sshFxpSetstatPacket).respond.

This change was to prevent the following scenario: toLocalPath(s.workDir, toLocalPath(s.workDir, p.Path)) which happens in (*sshFxpOpenPacket).respond because of this assignment to p.Path. This becomes a problem when workDir is being used and we're not checking filepath.IsAbs, resulting in double workDir prefix on Windows and Plan 9.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ach so! You’re absolutely right. The toLocalPath really should be deferred in that case until the .respond happens. Nice bug catch!

request_test.go Outdated Show resolved Hide resolved
@mafredri
Copy link
Contributor Author

@puellanivis thanks for the thorough PR review! I've amended most of the feedback now. The only thing I have yet to revert is #528 (comment) because doing so may introduce a bug when used with a workDir.

I went ahead with refactoring toLocalPath to be a method on Server. This also moved the code out of request_* files which seems appropriate, considering it's not used by the RequestServer.

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

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

Looking nice. 👍

server_plan9.go Outdated Show resolved Hide resolved
server_unix_test.go Outdated Show resolved Hide resolved
server_unix_test.go Outdated Show resolved Hide resolved
server_windows_test.go Outdated Show resolved Hide resolved
@puellanivis puellanivis merged commit 7da137a into pkg:master Oct 18, 2022
@mafredri mafredri deleted the mafredri/add-workdir-to-server branch October 19, 2022 09:15
kylecarbs pushed a commit to coder/coder that referenced this pull request Oct 21, 2022
* fix: Start SFTP sessions in user home (working directory)

This commit switches to our fork of `pkg/sftp` which includes a Server
option for changing the current working directory.

Attempt to upstream: pkg/sftp#528

Supercedes and closes #4420

Fixes #3620

* Update fork
@prochac
Copy link

prochac commented Oct 25, 2022

Cheers guys, It's being awesome for me to come just a week after this feature has been landed. I was seeking for it, it's not in release yet, and found this. Many thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants