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

fix(services/sftp): change default root config to remote server setting #2431

Merged
merged 4 commits into from
Jun 10, 2023
Merged

fix(services/sftp): change default root config to remote server setting #2431

merged 4 commits into from
Jun 10, 2023

Conversation

silver-ymz
Copy link
Member

Previously, sftp root config only supports absolute path. This patch is mentioned in #2192.

Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@Xuanwo
Copy link
Member

Xuanwo commented Jun 7, 2023

It is intended that the root path must be an absolute path. User must specify the correct root path for sftp services. But we can use the server's path as default.

If users doesn't set root, we will use the user's home directory as root.

We can mention this in root config. Please also add a new test job for this case (root is empty).

@Xuanwo Xuanwo marked this pull request as draft June 9, 2023 09:01
Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@silver-ymz
Copy link
Member Author

Update:

  • change default root to remote server setting.
  • add a new test job for it

The default root path in sftp can be set by both ChrootDirectory and sftp-server argument. And ChrootDirectory will change the apparent root directory, which means the program can't access files and commands outside that environmental directory tree. If ChrootDirectory is set to /home/foo, the previous implementation will create /home/foo/home/foo. I think it's an inappropriate behavior. So, in current implementation, it will do nothing if the user don't set root config.

About the github action test, because atmoz/sftp limits users to write the root directory, I have to modify /etc/ssh/sshd_config to change its initial entry directory.

Could you make a review about it?

@silver-ymz silver-ymz marked this pull request as ready for review June 9, 2023 16:21
@silver-ymz silver-ymz changed the title fix(services/sftp): support relative path for root config fix(services/sftp): change default root config to remote server setting Jun 9, 2023
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Mostly LGTM!

.github/workflows/service_test_sftp.yml Outdated Show resolved Hide resolved
core/src/services/sftp/docs.md Show resolved Hide resolved
Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@silver-ymz
Copy link
Member Author

All review suggestions have accepted. Could you make the final review?

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

@Xuanwo Xuanwo merged commit 412cf2b into apache:main Jun 10, 2023
@silver-ymz silver-ymz deleted the fix/relative-root-path-support branch June 13, 2023 09:37
@PsiACE PsiACE mentioned this pull request Jun 27, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants