mwilliamson / spur.py

Run commands and manipulate files locally or over SSH using the same interface
BSD 2-Clause "Simplified" License
267 stars 37 forks source link

Spur SshShell expects to be connected to a bash(like) session #26

Closed carlsverre closed 9 years ago

carlsverre commented 9 years ago

Right now when you connect to a machine with a custom shell (which maybe is there to reject connections), spur's spawn function does not take into account that the shell may not be bash. In my testing this results in raising an exception in _read_int_line as spur tries to parse the pid or which command output (all this in spur/ssh.py).

This came up for us when we tried using spur to connect to an ec2 machine with the root user. The AMI in question had a custom shell which essentially looks like this (my version not theirs, but it accomplishes the same thing):

#!/bin/bash
echo 'Please login as the user "ubuntu" rather than the user "root".'
exit 1

Since the connection successfully opens, spur goes ahead and runs the command and then fails to parse the rejection line as an int and raises a ValueError:

ValueError: invalid literal for int() with base 10: b'Please login as the user "ubuntu" rather than the user "root".'

I am not sure what the correct fix is here, but it seems that somehow validating that the receiving process is /bin/bash (or a bash variant) would be good.

For now we are catching the ValueError in a spawn wrapper, but this is not ideal since it may mask other real ValueErrors in the future.

Thanks!

mwilliamson commented 9 years ago

Hello! Have you tried using shell_type=spur.ssh.ShellTypes.minimal as an argument to spawn?

carlsverre commented 9 years ago

Unfortunately we can't use that shell type since we have a dependency on store_pid and getting a CommandNotFound exception. A potential fix would be to add something like "echo $0" to the beginning of the command string and asserting that the shell is one of a list of supported shells. If that would be fine with you I can submit a pull req.

mwilliamson commented 9 years ago

Unfortunately we can't use that shell type since we have a dependency on store_pid and getting a CommandNotFound exception.

Ah, fair enough.

I'm not quite sure I follow the behaviour that you want, though. Are you saying that you log in, you get the message to log in as ubuntu rather than root, and then you execute commands anyway?

carlsverre commented 9 years ago

The behavior I want is that when connecting via Spur to a machine with a non-standard shell (in this case a shell which prints an error message and then exits which prevents any further commands), for spur to not crash with a ValueError.

Here is a repro you can do on any linux machine:

Create an executable file somewhere with the following contents

#!/bin/bash
echo 'Please login as the user "ubuntu" rather than the user "root".'
exit 1

Create a user who uses the above file as it's shell

sudo useradd foobar
sudo passwd foobar
sudo chsh --shell /path/to/above/file foobar

SSH into the machine using spur and the foobar user and you will get the ValueError.

mwilliamson commented 9 years ago

The behavior I want is that when connecting via Spur to a machine with a non-standard shell (in this case a shell which prints an error message and then exits which prevents any further commands), for spur to not crash with a ValueError.

I'm still not quite sure what behaviour you do want (rather than the behaviour you don't want). Are you saying that you'd like an error to be raised, but not a ValueError?

carlsverre commented 9 years ago

Ideally a specific error is raised instead of a ValueError since a ValueError is ambiguous as to what it might entail. Something like "UnsupportedShellError" with a helpful error message.

mwilliamson commented 9 years ago

As of 0.3.14, the error spur.CommandInitializationError gets raised if the output parsing fails during command initialization. I went for that rather than the more specific UnsupportedShellError since that's not guaranteed to be the problem (but the error message for CommandInitializationError does suggest an unsupported shell is the most likely cause).

carlsverre commented 9 years ago

Thanks! That seems like good way to do it. Cheers