search

bramvdbogaerde / go-scp

Simple Golang scp client
Mozilla Public License 2.0
424 stars 89 forks source link

ParseResponse doesn't handle error responses correctly #55

Closed carlgreen closed 2 years ago

carlgreen commented 2 years ago

I am not sure that ParseResponse is correctly (or sufficiently) interpreting the response. This is based on observing the data sent over SCP using verbose mode, and looking at the OpenSSH implementation at scp.c

My reading of ParseResponse is that it expects the first byte of the response from the remote system to be an integer, and considers this the response type. If the response type is greater than 0 it considers the remainder of the line to be the response message. ParseFileInfos then takes this message and splits it in three. This gives the permissions, size, and filename.

I have run a couple of tests, running something like: scp -v user@host.local:c:/afile . scp -v user@host.local:c:/notafile . where only /afile exists

For the first command the local SCP process prints Sink: C0666 754880 afile, and for the second it prints Sink: \001scp: c:/notafile: No such file or directory.

Adding some debug statements to this library I see the same data being read in ParseResponse. In the first case it reads the first byte, 'C', as 0x43. As this is greater than 0 it saves the remainder — 0666 754880 afile — as the response message. This can be parsed by ParseFileInfos as expected. In the second case it reads the first byte \001 as the integer 1, so the remainder — scp: c:/notafile: No such file or directory — is saved as the response message. ParseFileInfos then splits the message by space and fails to convert the second word "c:/notafile:" to an integer.

When this problem occurs being called from CopyFromRemotePassThru it seems to hang waiting on the waitgroup, which I think is what is reported in #54.

Looking at the OpenSSH SCP implementation it appears to check if the first byte is 1 or 2 (L1645), and if so considers this an error. Otherwise I think for a file it expects the first byte to be 'C' (L1695). I think ParseResponse should consider the various possible values of the first byte (1, 2, 'C', 'D', 'E', 'T') and take action (possibly just unsupported error for some for now) based on that rather than just if it is non-zero. Then I think that the IsOk, IsWarning, and IsError functions also need to be adjusted — for example when the first byte is 'C', which is normal, IsFailure will return true. Finally, after ParseResponse and before ParseFileInfos maybe the failure state of the response should be checked?

carlgreen commented 2 years ago

Also if you would be happy for me to work something up and submit a pull request I can see what I can come up with.

bramvdbogaerde commented 2 years ago

Thanks for opening this issue. I welcome any contribution :)

bramvdbogaerde commented 2 years ago

@carlgreen IsFailure now checks if it is a warning or an error (as implemented in PR #56 ) If I understood your description correctly, this should fix the problem with considering "C" as a failure.

The PR also propagates the error back through the error channel in the client.go CopyFromRemote function. I believe that these improvements combined correctly fix the issue, and prevent ParseFileInfos from being called in case of a failure.

If you find some other issue with this code, this issue can be re-opened.