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

[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.

[davidsarah]

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.

[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.

[davidsarah]

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.

[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.

[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.

[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.

[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.

[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

[kevan]

Looks good; +r from me.

[david-sarah@…]

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