Skip to content

Let SFTP server figure out remote users home directories#24254

Merged
capnspacehook merged 1 commit intomasterfrom
capnspacehook/fix/23593
Apr 14, 2023
Merged

Let SFTP server figure out remote users home directories#24254
capnspacehook merged 1 commit intomasterfrom
capnspacehook/fix/23593

Conversation

@capnspacehook
Copy link
Copy Markdown
Contributor

@capnspacehook capnspacehook commented Apr 6, 2023

Previously a Teleport client using SFTP would resolve remote host user
home directories by making a subsystem request to a Teleport server
which would return the home directory. The problem was the subsystem
request counted as an open session, which could make the SFTP file
transfer fail. This was frustrating and didn't make much sense, but
after reading the SFTP specification again I realized that SFTP servers
are to handle relative paths by assuming they start at the user's home
directory. So let the server figure out the correct path and remove any
tilde prefixes from remote paths.

Fixes #23593.

@capnspacehook capnspacehook added backport/branch/v10 sftp Issues related to Teleport's SFTP implementation labels Apr 6, 2023
@github-actions github-actions Bot requested review from espadolini and xacrimon April 6, 2023 23:01
@capnspacehook capnspacehook force-pushed the capnspacehook/fix/23593 branch from 211fb55 to 2a44ee5 Compare April 6, 2023 23:13
Copy link
Copy Markdown
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Why a new channel for this?

The SSH protocol supports sending requests within a channel and over a global channel. We use that approach in other places where we need to get a string back (like version information).

https://github.com/gravitational/teleport/blob/master/lib/reversetunnel/srv.go#L1242
https://github.com/gravitational/teleport/blob/master/lib/reversetunnel/agent.go#L461

That feels like a better fit than creating a whole new channel for a string. What am I missing?

@espadolini
Copy link
Copy Markdown
Contributor

What does opensshd sftp do when connecting to openssh sshd, to resolve ~?

+1 for using a global request rather than opening a channel. It might make even more sense as a request local to the sftp subsystem channel, but sadly those can't carry a payload back so you'd need to either always blindly send it from the server side (and just wait with a timeout on the client side) or do some awkward back and forth with channel requests that are technically unrelated, and for a long time we've just been killing the channel whenever the server side received an unknown channel request (yay for extensibility, huh) so we can't really extend that part of the protocol until that's fixed and we pass a couple of major versions.

@capnspacehook capnspacehook force-pushed the capnspacehook/fix/23593 branch from 2a44ee5 to b59258e Compare April 11, 2023 15:51
@capnspacehook capnspacehook changed the title Use a SSH channel instead of a subsystem to return host user home dirs Let SFTP server figure out remote users home directories Apr 11, 2023
@capnspacehook capnspacehook force-pushed the capnspacehook/fix/23593 branch from b59258e to d49a703 Compare April 11, 2023 15:57
@capnspacehook
Copy link
Copy Markdown
Contributor Author

@russjones @espadolini thank you both for the questions and feedback, you both made me realize this was not an optimal solution at all. After reading the SFTP spec again I realized there's a much simpler solution, let the server figure out the correct path instead.

@capnspacehook capnspacehook requested a review from russjones April 11, 2023 16:01
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
@capnspacehook capnspacehook force-pushed the capnspacehook/fix/23593 branch from d49a703 to 6fa37ff Compare April 12, 2023 22:56
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Previously a Teleport client using SFTP would resolve remote host user
home directories by making a subsystem request to a Teleport server
which would return the home directory. The problem was the subsystem
request counted as an open session, which could make the SFTP file
transfer fail. This was frustrating and didn't make much sense, but
after reading the SFTP specification again I realized that SFTP servers
are to handle relative paths by assuming they start at the user's home
directory. So let the server figure out the correct path and remove any
tilde prefixes from remote paths.
Comment thread lib/sshutils/sftp/sftp.go
@capnspacehook
Copy link
Copy Markdown
Contributor Author

Friendly ping @xacrimon

@capnspacehook capnspacehook added this pull request to the merge queue Apr 14, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 14, 2023
@capnspacehook capnspacehook added this pull request to the merge queue Apr 14, 2023
Merged via the queue into master with commit 61f99cd Apr 14, 2023
@capnspacehook capnspacehook deleted the capnspacehook/fix/23593 branch April 14, 2023 16:39
@r0mant
Copy link
Copy Markdown
Collaborator

r0mant commented Apr 19, 2023

@capnspacehook Can you backport this to branch/v12 as soon as you can please?

This was referenced Apr 19, 2023
rosstimothy added a commit that referenced this pull request Jan 24, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
resulted in a weird UX and burried the underlying error in a wall
of text. In order to make it very apparent to tusers that this is
not a supported case these errors are now wrapped in a NonRetryableError
so that users can better understand what is happening.

Updates #22886.
rosstimothy added a commit that referenced this pull request Jan 24, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
rosstimothy added a commit that referenced this pull request Jan 28, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
rosstimothy added a commit that referenced this pull request Jan 29, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
github-merge-queue Bot pushed a commit that referenced this pull request Jan 29, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
github-actions Bot pushed a commit that referenced this pull request Jan 29, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
rosstimothy added a commit that referenced this pull request Jan 30, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
rosstimothy added a commit that referenced this pull request Jan 30, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
rosstimothy added a commit that referenced this pull request Jan 30, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
rosstimothy added a commit that referenced this pull request Jan 30, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
github-merge-queue Bot pushed a commit that referenced this pull request Jan 30, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
github-merge-queue Bot pushed a commit that referenced this pull request Jan 30, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
github-merge-queue Bot pushed a commit that referenced this pull request Jan 30, 2025
In #24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates #22886.
carloscastrojumo pushed a commit to carloscastrojumo/teleport that referenced this pull request Feb 19, 2025
…onal#51464)

In gravitational#24254 expansion
of tilde prefixes(i.e. `tsh scp CHANGELOG.md root@host:~foo/`) was
explicitly denied. However, because the error returned to the user
is a trace.BadParameter error the reauthentication logic kicks in
and attempts to resolve the issue with fresh credentials. This
error is now caught and wrapped in a NonRetryableError to prevent
the authentication logic from providing a weird UX.

Updates gravitational#22886.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sftp Issues related to Teleport's SFTP implementation size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tsh scp with ~ fails when max_session is set to 1

5 participants