execCommand support

[ruivapps]

it would be great if you can add in the support for execCommand
below is the output from git diff

diff --git a/MockSSH.py b/MockSSH.py
index e5f5113..18af0bf 100755
--- a/MockSSH.py
+++ b/MockSSH.py
@@ -317,7 +317,27 @@ class SSHAvatar(avatar.ConchUser):
         return None
 
     def execCommand(self, protocol, cmd):
-        raise NotImplemented
+        if cmd:
+            self.client = TransportWrapper(protocol)
+
+            cmd_and_args = cmd.split()
+            cmd, args = cmd_and_args[0], cmd_and_args[1:]
+            func = self.get_exec_func(cmd)
+
+            if func:
+                try:
+                    func(*args)
+                except Exception as e:
+                    self.client.write("Error: {0}".format(e))
+            else:
+                self.client.write("No such command.")
+
+            self.client.loseConnection()
+            protocol.session.conn.transport.expectedLoseConnection = 1
+
+        #raise NotImplemented
+    def get_exec_func(self, cmd):
+        return getattr(self, 'exec_' + cmd, None)
 
     def closed(self):
         pass
@@ -511,6 +531,29 @@ def startThreadedServer(commands,
 def stopThreadedServer():
     reactor.callFromThread(reactor.stop)
 
+class TransportWrapper(object):
+
+    def __init__(self, p):
+        self.protocol = p
+        p.makeConnection(self)
+        self.closed = False
+
+    def write(self, data):
+        self.protocol.outReceived(data)
+        self.protocol.outReceived('\r\n')
+
+        # Mimic 'exit' for the shell test
+        if '\x00' in data:
+            self.loseConnection()
+
+    def loseConnection(self):
+        if self.closed:
+            return
+
+        self.closed = True
+        self.protocol.inConnectionLost()
+        self.protocol.outConnectionLost()
+        self.protocol.errConnectionLost()
 
 if __name__ == "__main__":
     users = {'root': 'x'}

[ncouture]

Same as #18 (comment)

---

Thanks for taking the time to share your improvements on MockSSH @ruivapps .

Would you be able to write a test to support this feature?

Finally, would you be able to submit a pull request for inclusion? (see documentation for details)

---

It's worth noting that MockSSH's tests are not well implemented. Some depend on MockSSH's threadedServer which is a no-no for testing Twisted applications (see #5 ). That said, I would like to make sure new features are tested at least for functionality (end to end test, see the tests directory for examples).

[ruivapps]

can you add me to the repo so I can push branch for you to review?
currently I'm getting following error when trying to push branch to repo.

ERROR: Permission to ncouture/MockSSH.git denied to ruivapps.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

[ruivapps]

I push the branch to my GitHub for now
https://github.com/ruivapps/MockSSH
it's on exec_command_17 branch
you can pull the branch from my repo then merge it into MockSSH (so you don't need to add me to the project)

[ncouture]

@ruivapps apologies for the late reply. I will be following Github notifications more closely in the future.

I have opened the pull request and have no objection to add you as a contributor but would like to find out if I can help you resolve this issue by using Github's pull request feature that is outside of git first.

You can create a test pull requests on the project.

[ruivapps]

Sorry, I'm not really GitHub user. I didn't know I can do pull request directly from another repo. Thanks for sharing the tips!

[harbaum]

I've tried this patch and it mostly works as expected. I added a command to SSHAvatar like

   def exec_hello(self):
        self.client.write("HELLO WORLD")

But when i try to use it, the connection stays open:

$ ssh root@test hello
root@test's password: 
HELLO WORLD

Ssh sits there and waits. I've tried calling self.protocol.session.conn.transport.loseConnection() to TransportWrapper. This results in a closed connection but not the way it should be:

$ ssh root@test hello
root@test's password: 
Received disconnect from test port 22:10: user closed connection
Disconnected from test port 22

Any idea to handle this correctly?

[ruivapps]

did have much time this week. I just took a look on it, I have no idea what's the correct way to handle it.
it's been long time since I put the patch in, and I didn't even test the close connection behavior.

also, I am very very new to twisted, I mean brand new. This patch is the first time (and only time) I use twisted.

[harbaum]

Maybe i should take another look myself. I found that cowrie handles it correctly. But mockssh is a so much simpler setup ...

[ncouture]

@ruivapps I had a few questions about your implementation, notably regarding the non-termination of connections for non-interactive (exec command) sessions after command execution but also about the purpose and/or need of the added TransportWrapper and WriteLn classes.

It looks like the command execution logic added to SSHAvatar.execCommand might be duplicated to some extent so I would like to see with you if we can use a simpler implementation that continues to address to your use case, as per your tests.

[ruivapps]

it's very possible I added duplicated logic to the code. I don't remember why I use TransportWrapper and WriteLn, maybe because I am complete new to twisted, and also to be honest, I didn't even read the current mockssh code when I add that in. I was kind of in the hurry need it, then I see the execCommand was "raise NotImplementedError", i just tried to see if I can add it in. (one of our code use paramiko execcommand, and I really want that code unittested) So I spent half of the after noon and somehow hacked it working.

if the current implementation do not work with non-termination of connections for non-interactive session, then I would suggest to not merge it in. (if merged it, we should put a warning so people understand the unexpected behavior )

Maybe I had special case, it seems not having exec command wasn't an issue for most users. even for my case, we update our code later so we no longer use exec command (and we use official mockssh for unittest)

[ncouture]

The only criterias for inclusion we have right now is that this functionality results in the same behavior than using "exec command" on a standard OpenSSH server, which I think might be the right thing to do (for now).

I see two problems in your implementation that I failed to fix (more below):

• executing ssh server <command> manually does not terminate connection after exec
• the paramiko tests are passing

and I ended up with even more issues when trying to resolve the aformentioned (more below):

• executing ssh server <command> manually execs cmd and terminate conn, but ssh retcode is 255
  (status 11, see RFC 4253 section 11.1)
• the paramiko tests are failing due to connection lost (likely a result of the process protocol result causing the previous issue)

Could you try the following and confirm if these changes results in, SSH session/connection termination upon "exec command" execution?

1. From @ruivapps latest master branch, create a new branch (e.g.: "ssh-exec")
2. Add this file as examples/mock_ssh_exec.py:

#!/usr/bin/env python
#

import sys
import MockSSH

from twisted.python import log


users = {'admin': 'x'}

def exec_successful(instance):
    instance.writeln("ok")

def exec_failure(instance):
    instance.writeln("failure")


command = MockSSH.ArgumentValidatingCommand(
    'ls',
    [exec_successful],
    [exec_failure],
    *["123"])


if __name__ == "__main__":
    log.startLogging(sys.stderr)
    
    MockSSH.runServer(
        [command],
        prompt="hostname>",
        interface="localhost",
        port=9999,
        **users)

3. Apply this patch to 'tests/test_mock_execCommand.py':

diff --git a/tests/test_mock_execCommand.py b/tests/test_mock_execCommand.py
index 9fa8ca7..79a4058 100644
--- a/tests/test_mock_execCommand.py
+++ b/tests/test_mock_execCommand.py
@@ -25,7 +25,7 @@ def recv_all(channel):
 class TestParamikoExecCommand(unittest.TestCase):
 
     def setUp(self):
-        users = {'admin': 'x'}
+        users = {'testadmin': 'x'}
         command = MockSSH.ArgumentValidatingCommand(
                 'ls',
                 [exec_successful],
@@ -42,20 +42,27 @@ class TestParamikoExecCommand(unittest.TestCase):
         MockSSH.stopThreadedServer()
 
     def test_exec_command(self):
-        """test paramiko exec_commanbd
+        """test paramiko exec_command
         """
+        ssh = paramiko.SSHClient()
+        ssh.set_missing_host_key_policy(paramiko.WarningPolicy())
+        #ssh.connect('127.0.0.1', username='testadmin', password='x', port=9999)
+    
         ssh = paramiko.Transport(('127.0.0.1', 9999))
-        ssh.connect(username='admin', password='x')
+        ssh.connect(username='testadmin', password='x')
+        #import pdb
+        #pdb.set_trace()
         ch=ssh.open_session()
         ch.exec_command('ls')
         stdout = recv_all(ch)
+        raise Exception(stdout)
         self.assertEqual(stdout.strip(), 'failure')
-        ch=ssh.open_session()
-        ch.exec_command('ls 123')
-        stdout = recv_all(ch)
-        self.assertEqual(stdout.strip(), 'ok')
-        ch.close()
-        ssh.close()
+        # ch=ssh.open_session()
+        # ch.exec_command('ls 123')
+        # stdout = recv_all(ch)
+        # self.assertEqual(stdout.strip(), 'ok')
+        # ch.close()
+        # ssh.close()
 
 if __name__ == "__main__":
     unittest.main()

4. Apply this patch to MockSSH.py:

diff --git a/MockSSH.py b/MockSSH.py
index e8b0681..22488d9 100755
--- a/MockSSH.py
+++ b/MockSSH.py
@@ -135,9 +135,11 @@ class ArgumentValidatingCommand(SSHCommand):
 
 class SSHShell(object):
 
-    def __init__(self, protocol, prompt):
+    def __init__(self, protocol, prompt, interactive=True):
+        # self.closed = False
         self.protocol = protocol
         self.protocol.prompt = prompt
+        self.interactive = protocol.interactive
         self.showPrompt()
         self.cmdpending = []
 
@@ -161,7 +163,17 @@ class SSHShell(object):
                 self.showPrompt()
 
         if not len(self.cmdpending):
-            self.showPrompt()
+            if self.interactive:
+                self.showPrompt()
+            else:
+                # if self.closed:
+                #    return
+                self.protocol.terminal.transport.loseConnection()
+                # self.closed = 1
+                # self.protocol.inConnectionLost()
+                # self.protocol.outConnectionLost()
+                # self.protocol.errConnectionLost()
+
             return
 
         line = self.cmdpending.pop(0)
@@ -204,7 +216,8 @@ class SSHShell(object):
         self.runCommand()
 
     def showPrompt(self):
-        self.protocol.terminal.write(self.protocol.prompt)
+        if self.interactive:
+            self.protocol.terminal.write(self.protocol.prompt)
 
     def ctrl_c(self):
         self.protocol.lineBuffer = []
@@ -221,10 +234,11 @@ class SSHProtocol(recvline.HistoricRecvLine):
         self.commands = commands
         self.password_input = False
         self.cmdstack = []
+        self.interactive = True  # shell or ssh exec
 
     def connectionMade(self):
         recvline.HistoricRecvLine.connectionMade(self)
-        self.cmdstack = [SSHShell(self, self.prompt)]
+        self.cmdstack = [SSHShell(self, self.prompt, interactive=self.interactive)]
 
         transport = self.terminal.transport.session.conn.transport
         transport.factory.sessions[transport.transport.sessionno] = self
@@ -293,6 +307,29 @@ class SSHProtocol(recvline.HistoricRecvLine):
     def handle_CTRL_D(self):
         self.call_command(self.commands['_exit'])
 
+    def inConnectionLost(self):
+        print("inConnectionLost! stdin is closed! (we probably did it)")
+    def outConnectionLost(self):
+        print("outConnectionLost! The child closed their stdout!")
+        # now is the time to examine what they wrote
+        #print("I saw them write:", self.data)
+        # print("I saw %s lines" % lines)
+    def errConnectionLost(self):
+        print("errConnectionLost! The child closed their stderr.")
+
+
+class SSHTestProto(SSHProtocol):
+
+    def __init__(self, user, commands, command):
+        SSHProtocol.__init__(self, user, prompt=None, commands=commands)
+        self.interactive = False
+        self.command = command
+
+    def connectionMade(self):
+        SSHProtocol.connectionMade(self)
+        print 'Running exec command "%s"' % self.command
+        self.cmdstack[0].lineReceived(self.command)
+
 
 class SSHAvatar(avatar.ConchUser):
     implements(conchinterfaces.ISession)
@@ -316,28 +353,13 @@ class SSHAvatar(avatar.ConchUser):
     def getPty(self, terminal, windowSize, attrs):
         return None
 
-    def execCommand(self, protocol, cmd):
-        if cmd:
-            print 'CMD: %s' % cmd
-            self.client = TransportWrapper(protocol)
-
-            cmd_and_args = cmd.split()
-            cmd, args = cmd_and_args[0], cmd_and_args[1:]
-            #func = self.get_exec_func(cmd)
-            if cmd in self.commands:
-                if args == self.commands[cmd].required_arguments[1:]:
-                    print 'Command found (exec)', self.commands[cmd].required_arguments
-                    for x in self.commands[cmd].success_callbacks:
-                        x(WriteLn(self.client))
-                else:
-                    print "Command found but args not found (exec)"
-                    for x in self.commands[cmd].failure_callbacks:
-                        x(WriteLn(self.client))
-            else:
-                print "command not found: [%s] (exec)" %cmd
+    def execCommand(self, protocol, command):
+        serverProtocol = insults.SessionProtocol(SSHTestProto, self, self.commands, command)
 
-            self.client.loseConnection()
-            protocol.session.conn.transport.expectedLoseConnection = 1
+        serverProtocol.makeConnection(protocol)
+        protocol.makeConnection(session.wrapProtocol(serverProtocol))
+
+        protocol.session.conn.transport.expectedLoseConnection = 1
 
     def closed(self):
         pass
@@ -345,36 +367,6 @@ class SSHAvatar(avatar.ConchUser):
     def eofReceived(self):
         pass
 
-class WriteLn(object):
-    def __init__(self, client):
-        self.client = client
-
-    def writeln(self, data):
-        self.client.write(data)
-
-class TransportWrapper(object):
-
-    def __init__(self, p):
-        self.protocol = p
-        p.makeConnection(self)
-        self.closed = False
-
-    def write(self, data):
-        self.protocol.outReceived(data)
-        self.protocol.outReceived('\r\n')
-
-        # Mimic 'exit' for the shell test
-        if '\x00' in data:
-            self.loseConnection()
-
-    def loseConnection(self):
-        if self.closed:
-            return
-
-        self.closed = True
-        self.protocol.inConnectionLost()
-        self.protocol.outConnectionLost()
-        self.protocol.errConnectionLost()
 
 class SSHRealm:
     implements(portal.IRealm)

Results

Could you please confirm the following ?

1. Manualy executing command after example/mock_ssh_exec.py closes the connection after command is executed.

ssh admin@0 -p 9999 ls

2. Tests fail due to application losing connection.

Moving forward

To be clear; when using ssh exec command on an OpenSSH server, I expect the return code of ssh to be that of the command's execution (which might be the shell's return code, that should be the return code of the last command it executed).

For example (remote default shell is bash), the following should always print "0":

ssh hostname :
echo $?

and this should never print "0":

ssh hostname invalid-command
echo $?