From 75c24b0069bba6168fb9fe259fe1cb320c00e459 Mon Sep 17 00:00:00 2001
From: Tim Ross <tim.ross@goteleport.com>
Date: Fri, 24 Jan 2025 11:23:20 -0500
Subject: [PATCH] Prevent reauthentication for tilde prefix expansion errors

In https://github.com/gravitational/teleport/pull/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 https://github.com/gravitational/teleport/issues/22886.
---
 lib/client/client.go           | 13 ++++++++++++-
 lib/sshutils/sftp/sftp.go      | 16 +++++++++++++---
 lib/sshutils/sftp/sftp_test.go |  3 +--
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/lib/client/client.go b/lib/client/client.go
index 6d6d08f09f559..94284ed26494c 100644
--- a/lib/client/client.go
+++ b/lib/client/client.go
@@ -722,7 +722,18 @@ func (c *NodeClient) TransferFiles(ctx context.Context, cfg *sftp.Config) error
 	)
 	defer span.End()
 
-	return trace.Wrap(cfg.TransferFiles(ctx, c.Client.Client))
+	if err := cfg.TransferFiles(ctx, c.Client.Client); err != nil {
+		// TODO(tross): DELETE IN 19.0.0 - Older versions of Teleport would return
+		// a trace.BadParameter error when ~user path expansion was rejected, and
+		// reauthentication logic is attempted on BadParameter errors.
+		if trace.IsBadParameter(err) && strings.Contains(err.Error(), "expanding remote ~user paths is not supported") {
+			return trace.Wrap(&NonRetryableError{Err: err})
+		}
+
+		return trace.Wrap(err)
+	}
+
+	return nil
 }
 
 type netDialer interface {
diff --git a/lib/sshutils/sftp/sftp.go b/lib/sshutils/sftp/sftp.go
index b16eb656a3557..28f053ad10d53 100644
--- a/lib/sshutils/sftp/sftp.go
+++ b/lib/sshutils/sftp/sftp.go
@@ -329,7 +329,7 @@ func (c *Config) expandPaths(srcIsRemote, dstIsRemote bool) (err error) {
 		for i, srcPath := range c.srcPaths {
 			c.srcPaths[i], err = expandPath(srcPath)
 			if err != nil {
-				return trace.Wrap(err, "error expanding %q", srcPath)
+				return trace.Wrap(err)
 			}
 		}
 	}
@@ -337,13 +337,23 @@ func (c *Config) expandPaths(srcIsRemote, dstIsRemote bool) (err error) {
 	if dstIsRemote {
 		c.dstPath, err = expandPath(c.dstPath)
 		if err != nil {
-			return trace.Wrap(err, "error expanding %q", c.dstPath)
+			return trace.Wrap(err)
 		}
 	}
 
 	return nil
 }
 
+// PathExpansionError is an [error] indicating that
+// path expansion was rejected.
+type PathExpansionError struct {
+	path string
+}
+
+func (p PathExpansionError) Error() string {
+	return fmt.Sprintf("expanding remote ~user paths is not supported, specify an absolute path instead of %q", p.path)
+}
+
 func expandPath(pathStr string) (string, error) {
 	pfxLen, ok := homeDirPrefixLen(pathStr)
 	if !ok {
@@ -358,7 +368,7 @@ func expandPath(pathStr string) (string, error) {
 		return ".", nil
 	}
 	if pfxLen == 1 && len(pathStr) > 1 {
-		return "", trace.BadParameter("expanding remote ~user paths is not supported, specify an absolute path instead")
+		return "", trace.Wrap(PathExpansionError{path: pathStr})
 	}
 
 	// if an SFTP path is not absolute, it is assumed to start at the user's
diff --git a/lib/sshutils/sftp/sftp_test.go b/lib/sshutils/sftp/sftp_test.go
index 7be2b5ae0f9ab..85e203d3cbdab 100644
--- a/lib/sshutils/sftp/sftp_test.go
+++ b/lib/sshutils/sftp/sftp_test.go
@@ -34,7 +34,6 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	"github.com/gravitational/trace"
 	"github.com/stretchr/testify/require"
 
 	"github.com/gravitational/teleport/lib/utils"
@@ -585,7 +584,7 @@ func TestHomeDirExpansion(t *testing.T) {
 			name: "~user path",
 			path: "~user/foo",
 			errCheck: func(t require.TestingT, err error, i ...interface{}) {
-				require.True(t, trace.IsBadParameter(err))
+				require.ErrorIs(t, err, PathExpansionError{path: "~user/foo"})
 			},
 		},
 	}