Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle remote server closes channel without exit-status #71

Merged

Conversation

robertcheramy
Copy link
Collaborator

@robertcheramy robertcheramy commented Feb 26, 2024

Some ssh-server (APC Network Management Card AP9640) close the channel without sending an exit-status. This is correctly handled by openssh, so NET:SCP should also handle it.
I added two test cases: successful download without exit-status and interrupted download.

It would be great if you could review (and hopefully accept this PR), as it will help me to resove an issue on oxidized (ytti/oxidized#1802)

Some ssh-server (APC Network Management Card AP9640) close the channel
without sending an eof. This is correctly handled by openssh, so NET:SCP
should also handle it.
Added a test for this case.
The changed/added code passes Rubocop with default settings, with the
exception of the length of the test name, wich ich intended to be long.
lib/net/scp.rb Outdated
channel.on_close { |ch2| send("#{channel[:state]}_state", channel); raise Net::SCP::Error, "SCP did not finish successfully (#{channel[:exit]}): #{channel[:error_string]}" if channel[:exit] != 0 }
channel.on_close do
if channel[:exit].nil? && channel[:state] == :finish
# The remote closed the channel without sending an eof, but
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused, but is it eof or exit-status?

https://www.ietf.org/rfc/rfc4254.txt

6.10. Returning Exit Status

When the command running at the other end terminates, the following
message can be sent to return the exit status of the command.
Returning the status is RECOMMENDED. No acknowledgement is sent for
this message. The channel needs to be closed with
SSH_MSG_CHANNEL_CLOSE after this message.

The client MAY ignore these messages.

  byte      SSH_MSG_CHANNEL_REQUEST
  uint32    recipient channel
  string    "exit-status"
  boolean   FALSE
  uint32    exit_status

The remote command may also terminate violently due to a signal.
Such a condition can be indicated by the following message. A zero
'exit_status' usually means that the command terminated successfully.

  byte      SSH_MSG_CHANNEL_REQUEST
  uint32    recipient channel
  string    "exit-signal"
  boolean   FALSE
  string    signal name (without the "SIG" prefix)
  boolean   core dumped
  string    error message in ISO-10646 UTF-8 encoding
  string    language tag [RFC3066]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right. Not receiving SSH_MSG_CHANNEL_EOF is not the problem, I messed things up.

To clarify:
The remote implementation closes the channel without sending an eof nor an exit_status.
NET::SCP currently needs an exit_status = 0 to successfully end the transmission, which causes my problem.
As RFC4254 specifies sending exit_status als RECOMMENDED, NET::SCP should cope with not getting one.

I will correct my code and submit again.

And new test - download interrupted
@robertcheramy robertcheramy changed the title Handle remotes closes channel without eof Handle remotes closes channel without exit-status Feb 26, 2024
@robertcheramy robertcheramy changed the title Handle remotes closes channel without exit-status Handle remote server closes channel without exit-status Feb 26, 2024
@robertcheramy
Copy link
Collaborator Author

The code of the test does not pass rubocop tests, I will fix this.

@robertcheramy
Copy link
Collaborator Author

The code of the test does not pass rubocop tests, I will fix this.

This has been done with last commit, and this PR can be reviewed now. I will be happy to make any changes if needed.

@robertcheramy
Copy link
Collaborator Author

Thank you very much for approving this PR. Can someone merge it into master?

@mfazekas mfazekas merged commit 5ed6181 into net-ssh:master Apr 1, 2024
8 of 9 checks passed
@robertcheramy robertcheramy deleted the fix_remote_closes_without_eof branch December 13, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants