Concrete Minimal Shell Implementation

[Dannyb48]

Hi Michael,

I've been using the Spur library this past couple months to build some tools to remotely manage scaled out systems in our lab. It's a good tool, offers a high enough API that I don't need to work with Paramiko directly but low enough that I don't need the extra abstraction that Fabric offers.

I mostly work on Dell Platform and sometimes we need to log into the out-of-band embedded system management environment(DRAC/iDRAC) to manage and monitor the HW. I tried using the out-of-box ssh.MinimalShellType included with the library but I found that the iDRAC kept failing with command syntax problems. After some digging and troubleshooting I found that the MinimalShellType was still escaping the string being passed in

class MinimalShellType(object):
    supports_which = False
    
    def generate_run_command(self, command_args, store_pid,
            cwd=None, update_env={}, new_process_group=False):
        
        if store_pid:
            raise self._unsupported_argument_error("store_pid")
        
        if cwd is not None:
            raise self._unsupported_argument_error("cwd")
        
        if update_env:
            raise self._unsupported_argument_error("update_env")
        
        if new_process_group:
            raise self._unsupported_argument_error("new_process_group")
        
        return " ".join(map(escape_sh, command_args))
        
    
    def _unsupported_argument_error(self, name):
        return UnsupportedArgumentError("'{0}' is not supported when using a minimal shell".format(name))

When I printed out what the MinimalShellType class was generating for a command it was something like this:

'r' 'a' 'c' 'a' 'd' 'm' 'g' 'e' 't' 's' 'y' 's' 'i' 'n' 'f' 'o'

Which the on-board DRAC environment did not understand so it considered it a Syntax Error. To workaround it what I did was subclassed the ssh.MinimalTypeShell Class and created my own DracMinimalShellType class that overrides the generate_run_command() method. The only real change I made to that method was to NOT escape the string passed to the Shell class using the escape_sh(). It just passes it on through

class DracMinimalShellType(ssh.MinimalShellType):

    def generate_run_command(self, command_args, store_pid,
            cwd=None, update_env={}, new_process_group=False):

        if store_pid:
            raise self._unsupported_argument_error("store_pid")

        if cwd is not None:
            raise self._unsupported_argument_error("cwd")

        if update_env:
            raise self._unsupported_argument_error("update_env")

        if new_process_group:
            raise self._unsupported_argument_error("new_process_group")

        return command_args

That way what get's executed is:

"racadm getsysinfo"

Is it to be expected that the library is still escaping the string for MinimalShellTypes? If not, I can submit a pull request to fix that.

If it is, what is your stance on including concrete MinimalShellType implementations like I did above in the Spur Library? Or do you think that should stay higher level in the application using the Spur library? If we do include it the only other required change would be to include the concrete implementation in the ssh.ShellTypes class:

class ShellTypes(object):
    minimal = MinimalShellType()
    sh = ShShellType()
drac = DracMinimalShellType()

Looking forward to hearing what you think.

Thanks,

Danny

[mwilliamson]

Hmm, that's not the behaviour I'd expect for the minimal shell -- could I see what your invocation looks like?

[Dannyb48]

Sure, by the way my code runs from a Windows 10/ Windows 2012 Jump clients that run python 2.7.15 environment. I basically have a Wrapper/Bridge Class that wraps around the ShShell class to run the commands:

class EsxiNode(Node):

    def __init__(self, host=None, user=None, password=None, cmd=None, local=None, dest=None, shellType=None):
        self.__host = host
        self.__user = user
        self.__password = password
        self.__full_cmd = ['bin/sh', '-c']
        self.__shellType = shellType
        if shellType == "SH":
            self.__shell = ssh.SshShell(hostname=host, username=user, password=password, missing_host_key=ssh.MissingHostKey.accept)
            self.__shell_cmd = cmd
        elif shellType == "DRAC":
            #drac_min = DracMinimalShellType()
            min_shell = ssh.MinimalShellType()
            self.__shell = ssh.SshShell(hostname=host, username=user, password=password,
                                        missing_host_key=ssh.MissingHostKey.accept, shell_type=min_shell)
            self.__shell_cmd = cmd


    def run_cmd(self, shell_cmd=None):
        if self.__shellType == "DRAC":
            self.__shell_cmd = shell_cmd
            self.__full_cmd = self.__shell_cmd.split()
        else:
            self.__shell_cmd = shell_cmd
            self.__full_cmd.append(self.__shell_cmd)
        try:
            with self.__shell:
                result = self.__shell.run(self.__full_cmd, allow_error=True)
        except (NoSuchCommandError, CommandInitializationError, RunProcessError, Exception) as e:
            result = e.message
            return result
        else:
            return result

I essentially have a batch cmd script that wraps around the python script. On the command line you type:
msh exec "racadm getsysinfo"
which runs:
python msh.py exec "racadm getsysinfo"

I use argparse to get the command line parameters being passed at the windows command prompt, which passes it onto to a controller class which instantiates and executes the EsxiNode class I shared above.

I was doing some testing today printing out what the different Spur shell Types generate and this is what I see:

msh exec "racadm getsysinfo"
passed in cmdline: <type 'str'>

ShShell generated cmd: <type 'unicode'>

MinShell generated cmd: <type 'unicode'>

It looks like both shells are converting the commands from string obj to unicode string obj. Just curious for my understanding why is it necessary to do this unicode conversion?

[mwilliamson]

Unicode is used to make compatibility between Python 2 and 3 easier, and also to avoid encoding errors.

When you run a command, could you check the values of:

• self.__full_cmd (just before calling self.__shell.run)
• command_args (in generate_run_command)
• the return value of generate_run_command

[Dannyb48]

Thanks for the advice. So I added some print statements to check the values along the way. Here is the output of the commands as it passes a long the ShellType and ShShell Classes:

msh exec "racadm getsysinfo"
self.__full_cmd: ['racadm', 'getsysinfo']
MinimalShell command_args: ['racadm', 'getsysinfo']
MinimalShell generated_cmd: 'racadm' 'getsysinfo'
Spawn ShSell Command_in_cwd: 'racadm' 'getsysinfo'

cmdstat
        status       : 2
        status_tag   : COMMAND PROCESSING FAILED
        error        : 252
        error_tag    : COMMAND SYNTAX ERROR

To me it looks like the problem is how the command is being sent down the wire to iDRAC environment as 'racadm' 'getsysinfo'. If I log into the iDRAC and run the commands like how it's being escaped it returns the same error:

root@<iDRAC IP> password:
/admin1-> 'racadm' 'getsysinfo'
cmdstat
        status       : 2
        status_tag   : COMMAND PROCESSING FAILED
        error        : 252
        error_tag    : COMMAND SYNTAX ERROR

As a test I modified the escape_sh method in the ssh.py module to the following:

def escape_sh(value):
    #return "'" + value.replace("'", "'\\''") + "'"
    return value.replace("'", "'\\''")

Re-ran the command and it worked:

msh exec "racadm getsysinfo"
self.__full_cmd: ['racadm', 'getsysinfo']
MinimalShell command_args: ['racadm', 'getsysinfo']
MinimalShell generated_cmd: racadm getsysinfo
Spawn ShSell Command_in_cwd: racadm getsysinfo
<iDRAC IP output>:

RAC Information:
RAC Date/Time           = Wed Jul 11 13:43:37 2018

Firmware Version        = 3.15.15.15
Firmware Build          = 37
....

The command is being passed as a single unquoted string.

I know the iDRAC is a stripped down Linux that uses BusyBox but also implements the Server Management Command Line Protocol (SM CLP) Specification which is a general out-of-band hw management command prompt. From reading the Dell docs I believe is the command line prompt you are dropped into when you establish an SSH session to the iDRAC environment directly. I believe a lot of other Server HW platforms uses this as well (HP iLO/IBM IMM2) but I'm not 100% certain since it's been so long since I've worked on them. So I guess the questions would be:

Should Spur include a separate Base OobHwShellType class that mirrors the MinimalShellType class, or extends the MinimalShellType class and then have a separate implementation of escape_sh that does not place single quotes around the commands? At least this way the MinimalShellType is not altered in case people are using it successfully as is.

It would be nice if Spur offered this type of base functionality to user's out of the box?

[mwilliamson]

Thanks for the details. Just to clarify one of your earlier messages: when is 'r' 'a' 'c' 'a' 'd' 'm' 'g' 'e' 't' 's' 'y' 's' 'i' 'n' 'f' 'o' getting produced?

Does the shell you're describing support any escaping? Adding in another shell type, or parameterising the existing shell type, seems reasonable.

[Dannyb48]

So that seems to only get generated if I instantiate the ShellType classes myself like below but forget to split the string to an array. That was how I was initially doing my testing to try to understand what the ShellType classes were generating for a command. That's what tipped me off that the single quotes were what was causing the problem. I only realized that after I did the extra testing your were describing to do.

minshell = ssh.ShellTypes.minimal
print minshell.generate_run_command(command_args=args.cmd, store_pid=False, update_env={})


printed output:
'r' 'a' 'c' 'a' 'd' 'm' ' ' 'g' 'e' 't' 's' 'y' 's' 'i' 'n' 'f' 'o'

When I split the string properly like so print minshell.generate_run_command(command_args=args.cmd.split(), store_pid=False, update_env={}) I get the 'racadm' 'getsysinfo' as expected. So that was a user error on my part.

Yeah, it does this is the following regarding escaping from the SM CLP Specification:

Character
-----------
`       
Name
--------
escape character 

Description/Uses
-------------------
Escape character (the backquote character), used in front of
reserved characters to instruct the command parser to use the
reserved character without special meaning. When the escape
character is not followed by a reserved character, it is treated as a
normal character in the string that contains it. 

But me personally I've never had to escape anything. It's typically used to run simple verb commands with options for CRUD type operations regarding the Server HW. The command line processor is a simple message/response architecture. It's meant for simple actions nothing advanced.

[mwilliamson]

Hmm, the simplest thing might be to add an argument to MinimalShellType to allow customisation of the escaping. For instance, to completely disable escaping:

shell_type = MinimalShellType(escape=lambda arg: arg)

[Dannyb48]

Good point, I didn't think of that. I guess it would make the MinimalShellType Class look something like this?

class MinimalShellType(object):
    supports_which = False
    
    def __init__(self, escape=None):
        self._escape_sh = escape

    def generate_run_command(self, command_args, store_pid,
            cwd=None, update_env={}, new_process_group=False):

        if store_pid:
            raise self._unsupported_argument_error("store_pid")
        
        if cwd is not None:
            raise self._unsupported_argument_error("cwd")
        
        if update_env:
            raise self._unsupported_argument_error("update_env")
        
        if new_process_group:
            raise self._unsupported_argument_error("new_process_group")

        if self._escape_sh:
            return " ".join(map(self._escape_sh, command_args))
        else:
            return " ".join (map(escape_sh, command_args))
    
    def _unsupported_argument_error(self, name):
        return UnsupportedArgumentError("'{0}' is not supported when using a minimal shell".format(name))     

That would mean we would need to modify the ShellType class to take in this parameter as well?

class ShellTypes(object):
  def __init__(escape=None)

     if escape:
           minimal = MinimalShellType(escape=escape)
     else:
           minimal = MinimalShellType()
    sh = ShShellType()

Or do we leave that as is and have users that want the custom escaping to instantiate the MinimalShellType directly with the custom escape parameter and for the default use cases use ShellType to instantiate the minimal shell for you?

[mwilliamson]

I'd leave ShellTypes as-is, and allow the user to create their own MinimalShellType instance with a custom escaping function.

[Dannyb48]

Thanks! I'll file a Pull Request this weekend with the code changes.