Skip to content

fix: Start SFTP sessions in user home (working directory)#4420

Closed
mafredri wants to merge 3 commits into
mainfrom
mafredri/agent-sftp-dir
Closed

fix: Start SFTP sessions in user home (working directory)#4420
mafredri wants to merge 3 commits into
mainfrom
mafredri/agent-sftp-dir

Conversation

@mafredri

@mafredri mafredri commented Oct 7, 2022

Copy link
Copy Markdown
Member

This is a simple fix for #3620 by changing the working directory of
coder agent at runtime.

Since the agent doesn't write files to it's working directory, I can't
think of any downsides to this method. It does not, however, exclude
that we rewrite the SFTP implementation to use the modernized
RequestServer sometime in the future.

Opted to place this in agent init vs cli/agent because of portability
of fix, tests don't usually use cli/agent.

Fixes #3620

This is a simple fix for #3620 by changing the working directory of
`coder agent` at runtime.

Since the agent doesn't write files to it's working directory, I can't
think of any downsides to this method. It does not, however, exclude
that we rewrite the SFTP implementation to use the modernized
`RequestServer` sometime in the future.

Opted to place this in agent init vs `cli/agent` because of portability
of fix, tests don't usually use `cli/agent`.

Fixes #3620
@mafredri mafredri self-assigned this Oct 7, 2022
@mafredri mafredri requested a review from a team October 7, 2022 16:51

@deansheather deansheather left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this the only way to solve this problem? Can we not tell the sftp module to use a certain starting directory instead?

Programs changing working directory by themselves causes relative paths (if they're ever used) to break

@mafredri

mafredri commented Oct 7, 2022

Copy link
Copy Markdown
Member Author

Is this the only way to solve this problem? Can we not tell the sftp module to use a certain starting directory instead?

Programs changing working directory by themselves causes relative paths (if they're ever used) to break

Yeah, the alternative is discussed in #3620. But like Kyle mentions it's a quite involved. This would allow us to fix it immediately whereas if we go the alternative approach not sure when we'd get around to it.

Are there any relative paths used by the agent? I for one can't find any. The only file we write (to my knowledge) is a log file and that one is opened earlier and uses an absolute path.

@mafredri

mafredri commented Oct 8, 2022

Copy link
Copy Markdown
Member Author

@deansheather thinking about this some more, I failed to consider the effect this will have on tests. Even if nothing breaks today, there's bound to be some breakage (e.g. looking up testdata/) down the line.

Because of that, I would consider:

  1. Moving this cli/agent.go, disabling the chdir in test-mode and adding a hidden test flag to enable it for one particular test running the agent as a separate command
  2. Forking the sftp package and adding adding a flag for changing the initial directory (have not validated the how plausible this is)

@deansheather

Copy link
Copy Markdown
Member

We could also contribute it upstream

mafredri added a commit 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 closed this Oct 14, 2022
@mafredri mafredri deleted the mafredri/agent-sftp-dir branch October 14, 2022 10:49
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 14, 2022
mafredri added a commit 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
kylecarbs pushed a commit 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SFTP sessions start in /tmp/coder.KnYnxH (instead of $HOME)

2 participants