Fix scp regressions for 6.x #5729

a-palchikov · 2021-02-25T19:16:18Z

Use correct target directory path in error message.
Handle target directory/file renames properly.

I've only tested these changes manually and I was working on automatic test coverage for the rest of the use-cases but I'm not sure I can make it in time for rc.2.

Fixes #5695.

russjones · 2021-02-25T19:48:34Z

@andrejtokarcik Can you review this?

russjones · 2021-02-25T19:48:45Z

@a-palchikov If we can't make it to rc.2, let's aim for rc.3?

a-palchikov · 2021-02-25T19:59:54Z

Ok - I'll work on the tests to be on the safe side.

andrejtokarcik · 2021-02-25T22:40:55Z

lib/sshutils/scp/scp.go

+	if cmd.FileSystem.IsDir(cmd.Flags.Target[0]) {
+		// Copying into an existing directory? append to it:
+		st.push(fc.Name, st.stat)
+	} else {
+		// If target specifies a new directory, we need to reset
+		// state with it
+		st.path = newPathFromDirAndTimes(cmd.Flags.Target[0], st.stat)
+	}


I'm mentioning this here just so we're aware of the current behaviour not that it's necessary to change it, feel free to close this note with no action:

This could cause unexpected data duplication, especially in automated scripts which may simply rely on the fact that tsh scp always overwrites its target. For example:

$ stat /tmp/zz stat: cannot stat '/tmp/zz': No such file or directory $ tsh scp -r ./large-tree/ /tmp/zz/ [...] $ diff ./large-tree/ /tmp/zz/ && echo same tree same tree $ tsh scp -r ./large-tree/ /tmp/zz/ # re-run the same tsh scp command [...] $ diff ./large-tree/ /tmp/zz/ Only in /tmp/zz/: large-tree

The files of large-tree would end up in two copies under /tmp/zz: once directly under /tmp/zz, once under /tmp/zz/large-tree.

Yes, this is consistent with OpenSSH client's behavior - initial attempt to copy a directory into a new non-existing location causes a rename, but existing location appends to it.

I can't test it right now but IIRC, OpenSSH scp changes its dir-copying behaviour based on the trailing slash which I think tsh scp would not.

That could be possible - I'll test that as well.

client (ubuntu):

$ ssh -V OpenSSH_8.4p1, OpenSSL 1.1.1f 31 Mar 2020

server (centos):

$ ssh -V OpenSSH_7.4p1, OpenSSL 1.0.2k-fips 26 Jan 2017

Trailing slash does not change the behavior as described in the linked issue.

@a-palchikov I this code is always run on the client? Server process is sshd?

Yes, the server is sshd. It does not have a version flag but outputs the version anyways:

$ sshd -V unknown option -- V OpenSSH_7.4p1, OpenSSL 1.0.2k-fips 26 Jan 2017

andrejtokarcik · 2021-02-25T22:49:44Z

BTW I had to apply the following to make tsh scp work with zsh:

diff --git a/lib/srv/exec.go b/lib/srv/exec.go
index e8cf6232d..a822b0293 100644
--- a/lib/srv/exec.go
+++ b/lib/srv/exec.go
@@ -247,7 +247,7 @@ func (e *localExec) transformSecureCopy() error {
        if err != nil {
                return trace.Wrap(err)
        }
-       e.Command = fmt.Sprintf("%s scp --remote-addr=%s --local-addr=%s %v",
+       e.Command = fmt.Sprintf("%s scp --remote-addr=%q --local-addr=%q %v",
                teleportBin,
                e.Ctx.ServerConn.RemoteAddr().String(),
                e.Ctx.ServerConn.LocalAddr().String(),

The encountered error was:

zsh:1: no matches found: --remote-addr=[::1]:59236

a-palchikov · 2021-02-26T16:40:20Z

lib/sshutils/scp/scp_test.go

 			args := append(tt.args, source)

-			// Source is missing, expect an error.
-			err = runSCP(cmd, args...)


I factored this into a dedicated test - no need to test this for each local testcase.

a-palchikov · 2021-02-26T16:47:32Z

lib/sshutils/scp/local.go

@@ -49,7 +49,7 @@ func (l *localFileSystem) Chtimes(path string, atime, mtime time.Time) error {
 // MkDir creates a directory
 func (l *localFileSystem) MkDir(path string, mode int) error {
 	fileMode := os.FileMode(mode & int(os.ModePerm))
-	err := os.MkdirAll(path, fileMode)
+	err := os.Mkdir(path, fileMode)


This differed from OpenSSH's behavior which does not automatically create intermediate directories. If that's a concern, we can revert this change.

Adhering to OpenSSH behaviour seems more logical to me. When people type scp they expect it to behave just like scp does when it comes to things like this.

I agree, we should be as consistent as possible with OpenSSH.

@alex-kovoy Any reason you made this MkdirAll? #2055

@russjones That was original implementation, it appeared in my PR because I move some code around while working on HTTP transfers. See the original (deleted) scp.go file in that PR.

Hrm, interesting the original commit seems to indicate MkdirAll lines up with OpenSSH behavior: 80df1bb

The original commit that introduced this changes is 2ad2fb4 and it seems to be fixing this problem from #274.
Since the solution caused another issue, it was changed in c6c77a1 but os.MkdirAll remained.

As for the behavior of OpenSSH (7.4p1 from above) and the following source layout:

$ tree foo foo ├── foo1 └── foo-dir └── foo-file

Copying to a non-existing directory (zz, 1 level), renames the source into it (or creates it remotely and copies the contents of the source directory):

$ scp -r ./foo [email protected]:zz foo1 100% 0 0.0KB/s 00:00 foo-file 100% 0 0.0KB/s 00:00

with the resulting remote layout:

# tree zz zz ├── foo1 └── foo-dir └── foo-file

while opening into a directory with an intermediate non-existing directory:

$ scp -r ./foo [email protected]:non-existing/zz scp: non-existing/zz: No such file or directory

but it works with tsh in 5.2.1.

russjones · 2021-02-26T18:24:58Z

@xacrimon @andrejtokarcik Can you to re-review?

xacrimon · 2021-02-26T18:10:51Z

lib/sshutils/scp/local.go

@@ -49,7 +49,7 @@ func (l *localFileSystem) Chtimes(path string, atime, mtime time.Time) error {
 // MkDir creates a directory
 func (l *localFileSystem) MkDir(path string, mode int) error {
 	fileMode := os.FileMode(mode & int(os.ModePerm))
-	err := os.MkdirAll(path, fileMode)
+	err := os.Mkdir(path, fileMode)


Adhering to OpenSSH behaviour seems more logical to me. When people type scp they expect it to behave just like scp does when it comes to things like this.

Fixes #5695.

russjones

Bot.

russjones · 2021-02-27T01:23:24Z

@a-palchikov Just to clarify, we now match OpenSSH behavior right? I agree, I think that's what users expectation here is.

a-palchikov · 2021-02-28T15:54:18Z

@a-palchikov Just to clarify, we now match OpenSSH behavior right? I agree, I think that's what users expectation here is.

Yes, w.r.t to copying a directory to a remote path with intermediates that do not exist.

Handle target directory/file renames. Fixes #5695.

…ote location with intermediate directories

…bols that require escaping

Fixes #5695.

a-palchikov requested review from klizhentas, r0mant and russjones as code owners February 25, 2021 19:16

xacrimon added the test-plan-problem Issues which have been surfaced by running the manual release test plan label Feb 25, 2021

xacrimon approved these changes Feb 25, 2021

View reviewed changes

a-palchikov force-pushed the dmitri/5695-scp branch from 32d26d7 to ce424dd Compare February 25, 2021 19:57

andrejtokarcik approved these changes Feb 25, 2021

View reviewed changes

a-palchikov force-pushed the dmitri/5695-scp branch 2 times, most recently from 3a02ae8 to 3f8e3fb Compare February 26, 2021 16:34

a-palchikov commented Feb 26, 2021

View reviewed changes

a-palchikov requested review from xacrimon and andrejtokarcik February 26, 2021 17:10

xacrimon approved these changes Feb 26, 2021

View reviewed changes

andrejtokarcik approved these changes Feb 26, 2021

View reviewed changes

a-palchikov force-pushed the dmitri/5695-scp branch from 9ce6407 to 3796e10 Compare February 26, 2021 20:31

a-palchikov added a commit that referenced this pull request Feb 26, 2021

Backport of #5729 to v6.

5bc1d67

Fixes #5695.

a-palchikov mentioned this pull request Feb 26, 2021

[branch/v6] fix scp regressions #5764

Merged

russjones approved these changes Feb 27, 2021

View reviewed changes

a-palchikov added 4 commits March 2, 2021 19:38

Use correct target directory path.

f9eb35f

Handle target directory/file renames. Fixes #5695.

Check whether . is a base directory directly

701cba8

Add more test coverage for sink mode

a042a3c

Use non-greedy Mkdir variant and add a test-case for non-existing rem…

3b813c6

…ote location with intermediate directories

a-palchikov added 2 commits March 2, 2021 19:38

Quote the address arguments to avoid issues with formats that use sym…

cf92794

…bols that require escaping

Remove args as these can be deduced automatically

ebd0a26

a-palchikov force-pushed the dmitri/5695-scp branch from 8ff0b44 to ebd0a26 Compare March 2, 2021 18:38

a-palchikov added a commit that referenced this pull request Mar 2, 2021

Backport of #5729 to v6.

30034b1

Fixes #5695.

Merge branch 'master' into dmitri/5695-scp

70ddd83

russjones pushed a commit that referenced this pull request Mar 2, 2021

Backport of #5729 to v6.

70d0ec7

Fixes #5695.

russjones added 2 commits March 2, 2021 13:20

Merge branch 'master' into dmitri/5695-scp

95c5a8c

Merge branch 'master' into dmitri/5695-scp

b90fbbb

russjones merged commit ae4e5bf into master Mar 4, 2021

russjones deleted the dmitri/5695-scp branch March 4, 2021 01:49

webvictim mentioned this pull request May 5, 2021

ami auto branch 1620236164 #6745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scp regressions for 6.x #5729

Fix scp regressions for 6.x #5729

a-palchikov commented Feb 25, 2021

russjones commented Feb 25, 2021

russjones commented Feb 25, 2021

a-palchikov commented Feb 25, 2021

andrejtokarcik Feb 25, 2021 •

edited

Loading

a-palchikov Feb 26, 2021

andrejtokarcik Feb 26, 2021

a-palchikov Feb 26, 2021

a-palchikov Feb 26, 2021

russjones Mar 1, 2021

a-palchikov Mar 2, 2021

andrejtokarcik commented Feb 25, 2021

a-palchikov Feb 26, 2021

a-palchikov Feb 26, 2021

xacrimon Feb 26, 2021

webvictim Mar 1, 2021

russjones Mar 1, 2021

alex-kovoy Mar 1, 2021

russjones Mar 1, 2021

a-palchikov Mar 2, 2021

a-palchikov Mar 2, 2021

russjones commented Feb 26, 2021

xacrimon Feb 26, 2021

russjones left a comment

russjones commented Feb 27, 2021

a-palchikov commented Feb 28, 2021

Fix scp regressions for 6.x #5729

Fix scp regressions for 6.x #5729

Conversation

a-palchikov commented Feb 25, 2021

russjones commented Feb 25, 2021

russjones commented Feb 25, 2021

a-palchikov commented Feb 25, 2021

andrejtokarcik Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrejtokarcik commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones commented Feb 26, 2021

Choose a reason for hiding this comment

russjones left a comment

Choose a reason for hiding this comment

russjones commented Feb 27, 2021

a-palchikov commented Feb 28, 2021

andrejtokarcik Feb 25, 2021 •

edited

Loading