#1442 closed defect (fixed)

scp fails silently when copying to a Tahoe SFTP frontend; it should fail loudly

Reported by: davidsarah Owned by: davidsarah
Priority: major Milestone: 1.9.0
Component: code-frontend Version: 1.8.2
Keywords: scp error sftp reviewed Cc:
Launchpad Bug:

Description (last modified by davidsarah)

Our SFTP frontend does not support scp, which is fine (scp tries to open a shell over the SSH connection, rather than using SFTP, and the set of commands that it can issue to that shell is not well-defined we don't implement the protocol it uses).

However, it isn't fine that it fails silently. This is probably a bug in scp rather than Tahoe, since Tahoe does report an error to the client when it tries to open a shell or issue unknown commands: lines 1873 and 1888 here. (We need to accept shell connections in order to support the df command used by sshfs.) We might be able to get scp to fail loudly by reporting a different error, though.

Attachments (2)

fix-1442-and-1446-version2.darcs.patch (25.0 KB) - added by davidsarah at 2011-07-27T15:26:36Z.
SFTP: write an error message to standard error for unrecognized shell commands. Change the existing message for shell sessions to be written to standard error, and refactor some duplicated code. fixes #1442. Also includes the patch for #1446.
fix-1442-and-1446-version3.darcs.patch (37.2 KB) - added by davidsarah at 2011-07-30T00:32:01Z.
SFTP: write an error message to standard error for unrecognized shell commands. Change the existing message for shell sessions to be written to standard error, and refactor some duplicated code. Also change the lines of the error messages to end in CRLF, and take into account Kevan's review comments. fixes #1442, #1446

Download all attachments as: .zip

Change History (16)

comment:1 Changed at 2011-07-26T21:42:36Z by davidsarah

  • Owner changed from davidsarah to T_X

I can't reproduce this bug, reported on behalf of T_X. After setting up SFTP access on port 8022 with account name davidsarah, I get:

$ scp -oport=8022 test davidsarah@127.0.0.1:/
davidsarah@127.0.0.1's password: [password]
lost connection

The "lost connection" is a sufficient error indication.

The OpenSSH sftp client does work in the configuration I set up.

ssh also gives an error as intended:

$ ssh -p 8022 davidsarah@127.0.0.1
davidsarah@127.0.0.1's password: [wrong]
Permission denied, please try again.
davidsarah@127.0.0.1's password: [password]
This server supports only SFTP, not shell sessions.
                                                   Connection to 127.0.0.1 closed.

I'm using the scp and ssh from Ubuntu Maverick (OpenSSH_5.5p1 Debian-4ubuntu5, OpenSSL 0.9.8o 01 Jun 2010), and Tahoe trunk (with local changes that shouldn't be relevant to SFTP).

Assigning to T_X in case there is any difference in how we're using scp.

comment:2 follow-ups: Changed at 2011-07-27T04:18:17Z by T_X

Hmm, weird, I'm also using OpenSSH 5.5p1, but on Debian squeeze though... Ah wait, just found what's different: I'm copying _from_ tahoe-lafs _to_ a local directory. You're doing it the other way around and there I also get that "lost connection". However when copying from tahoe-lafs, there I don't get it:

$ scp -oport=8022 /tmp/8BP102.gif-2  root@127.0.0.1:/; echo $?
root@127.0.0.1's password: 
lost connection
1
$ scp -oport=8022 root@127.0.0.1:/music/8bitpeoples/8BP102.gif /tmp/; echo $?
root@127.0.0.1's password: 
1
$

And at least scp gives some sort of error return code, just no error message via stdout/stderr appears.

comment:3 in reply to: ↑ 2 Changed at 2011-07-27T04:28:53Z by davidsarah

  • Owner changed from T_X to davidsarah
  • Status changed from new to assigned

Replying to T_X:

$ scp -oport=8022 root@127.0.0.1:/music/8bitpeoples/8BP102.gif /tmp/; echo $?
root@127.0.0.1's password: 
1
$

And at least scp gives some sort of error return code, just no error message via stdout/stderr appears.

OK, I can reproduce that. Seems like a clear bug in scp. I will file a ticket with OpenSSH or find an existing one.

comment:4 in reply to: ↑ 2 Changed at 2011-07-27T15:25:54Z by davidsarah

Replying to T_X:

Ah wait, just found what's different: I'm copying _from_ tahoe-lafs _to_ a local directory. You're doing it the other way around and there I also get that "lost connection". However when copying from tahoe-lafs, there I don't get it: [...]

To avoid confusing other readers, you meant "when copying to tahoe-lafs, there I don't get [the error]".

Anyway, it turns out that scp was just expecting the server to write the error message. (I still think that's a bug, but never mind.)

With attachment:fix-1442-and-1446-version2.darcs.patch, we get:

$ scp -oport=8022 test davidsarah@127.0.0.1:/test
davidsarah@127.0.0.1's password: 
This server supports only the SFTP protocol.
It does not support SCP, interactive shell sessions, or commands other than 'df'.
lost connection

$ scp -oport=8022 davidsarah@127.0.0.1:/test test
davidsarah@127.0.0.1's password: 
This server supports only the SFTP protocol.
It does not support SCP, interactive shell sessions, or commands other than 'df'.

$ ssh -oport=8022 davidsarah@127.0.0.1
davidsarah@127.0.0.1's password: 
This server supports only the SFTP protocol.
It does not support SCP, interactive shell sessions, or commands other than 'df'.
Connection to 127.0.0.1 closed.

Changed at 2011-07-27T15:26:36Z by davidsarah

SFTP: write an error message to standard error for unrecognized shell commands. Change the existing message for shell sessions to be written to standard error, and refactor some duplicated code. fixes #1442. Also includes the patch for #1446.

comment:5 Changed at 2011-07-27T15:28:26Z by davidsarah

  • Keywords review-needed added
  • Milestone changed from undecided to 1.9.0
  • Owner davidsarah deleted
  • Status changed from assigned to new

comment:6 Changed at 2011-07-27T21:40:44Z by davidsarah

  • Summary changed from scp fails silently when copying from a Tahoe SFTP frontend; it should fail loudly to scp fails silently when copying to a Tahoe SFTP frontend; it should fail loudly

comment:7 Changed at 2011-07-29T00:28:48Z by kevan

  • Owner set to kevan
  • Status changed from new to assigned

comment:8 Changed at 2011-07-29T18:11:14Z by davidsarah

  • Description modified (diff)

comment:9 follow-up: Changed at 2011-07-29T22:04:21Z by kevan

Good patches. A few comments:

  • Should the second line sent to the client in ShellSession._unsupported say the exact df invocation that it supports (df -P -k /)? It'd be a bit confusing to a user that expected df used alone to work, since they'd see the same error message.
  • Should the tests test for the second line in ShellSession._unsupported?
  • Nitpick: the tests that look for the carriage returns you added only test for their presence somewhere in the output, even though we want the lines sent to the client to end with CRLFs. Making those tests more specific might detect, e.g., someone editing one of the two output lines for either df or ShellSession._unsupported and forgetting to add the CRLF at the end; as it is, the tests will pass so long as some line in the output retains its CRLF.

The changes work fine with the OpenSSH sftp and scp programs on my system. It'd be interesting to see what they do on a popular Windows program, like WinSCP, but I don't have a Windows box here.

comment:10 Changed at 2011-07-29T22:04:44Z by kevan

  • Keywords review-needed removed
  • Owner changed from kevan to davidsarah
  • Status changed from assigned to new

comment:11 in reply to: ↑ 9 Changed at 2011-07-30T00:28:30Z by davidsarah

Replying to kevan:

Good patches. A few comments:

  • Should the second line sent to the client in ShellSession._unsupported say the exact df invocation that it supports (df -P -k /)? It'd be a bit confusing to a user that expected df used alone to work, since they'd see the same error message.

It's a bit of an implementation detail that we accept that command (and we only do so because sshfs needs it). I've changed the text to:

This server supports only the SFTP protocol. It does not support SCP,
interactive shell sessions, or commands other than one needed by sshfs.
  • Should the tests test for the second line in ShellSession._unsupported?

I don't think it's necessary.

  • Nitpick: the tests that look for the carriage returns you added only test for their presence somewhere in the output, even though we want the lines sent to the client to end with CRLFs. Making those tests more specific might detect, e.g., someone editing one of the two output lines for either df or ShellSession._unsupported and forgetting to add the CRLF at the end; as it is, the tests will pass so long as some line in the output retains its CRLF.

Fixed in next patch.

The changes work fine with the OpenSSH sftp and scp programs on my system. It'd be interesting to see what they do on a popular Windows program, like WinSCP, but I don't have a Windows box here.

These changes should only matter for sshfs (which I've tested). WinSCP and most other clients don't depend on shell commands.

Changed at 2011-07-30T00:32:01Z by davidsarah

SFTP: write an error message to standard error for unrecognized shell commands. Change the existing message for shell sessions to be written to standard error, and refactor some duplicated code. Also change the lines of the error messages to end in CRLF, and take into account Kevan's review comments. fixes #1442, #1446

comment:12 Changed at 2011-07-30T00:32:55Z by davidsarah

  • Keywords review-needed added
  • Owner changed from davidsarah to kevan

comment:13 Changed at 2011-07-30T01:18:42Z by kevan

  • Keywords reviewed added; review-needed removed
  • Owner changed from kevan to davidsarah

Looks good; +r from me.

comment:14 Changed at 2011-07-30T03:08:20Z by david-sarah@…

  • Resolution set to fixed
  • Status changed from new to closed

In a2699ea6f635c992:

SFTP: write an error message to standard error for unrecognized shell commands. Change the existing message for shell sessions to be written to standard error, and refactor some duplicated code. Also change the lines of the error messages to end in CRLF, and take into account Kevan's review comments. fixes #1442, #1446

Note: See TracTickets for help on using tickets.