Open ruivapps opened 7 years ago
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).
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.
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)
@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.
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!
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?
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.
Maybe i should take another look myself. I found that cowrie handles it correctly. But mockssh is a so much simpler setup ...
@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.
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)
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):
ssh server <command>
manually does not terminate connection after exec and I ended up with even more issues when trying to resolve the aformentioned (more below):
ssh server <command>
manually execs cmd and terminate conn, but ssh retcode is 255
(status 11, see RFC 4253 section 11.1)Could you try the following and confirm if these changes results in, SSH session/connection termination upon "exec command" execution?
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
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()
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):
self.protocol = protocol
self.protocol.prompt = prompt
@@ -161,7 +163,17 @@ class SSHShell(object): self.showPrompt()
if not len(self.cmdpending):
self.showPrompt()
if self.interactive:
self.showPrompt()
else:
self.protocol.terminal.transport.loseConnection()
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!")
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:]
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):
-class TransportWrapper(object):
self.protocol.errConnectionLost()
class SSHRealm: implements(portal.IRealm)
### Results
Could you please confirm the following ?
example/mock_ssh_exec.py
closes the connection after command is executed.
ssh admin@0 -p 9999 ls
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 $?
I am agree the ssh exit code should match the command exit code. Testing stdout/stderr is incorrect/incomplete way to validate the exec feature. If command running successfully, by default the exit code of ssh itself should be 0 (otherwise it should treat as executing failed instead)
The patch I was trying to submit only consider if stdout/stderr are matching, but not counting the exit code. The implementation in patch will return incorrect return code.
Well said, and the same is true for MockSSH; it has no notion of command execution, all execution is mocked by named commands that can only do input/output (to some extent).
@ruivapps @harbaum What's your use case for exec command?
we used to have some code that use paramiko and exec_command. so I was using MockSSH for unittest. we already updated our code, and we no longer use paramiki/exec_command. We no longer need exec_command on MockSSH
I love this script. But I really need to get exec_command working for my unit testing. I tried to troubleshoot the problem, but came up empty. I've used ruivapps fix. It connects perfectly, but i get an exception when trying to execute a command.
My test from the client side is the following: From the client side, i am executing the following:
>>> c = paramiko.SSHClient()
>>> c.set_missing_host_key_policy(paramiko.AutoAddPolicy())
>>> c.connect('127.0.0.1',username='testadmin', password='x', port=1025)
>>> c.exec_command('pwd')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko/lib/python3.6/site-packages/paramiko/client.py", line 514, in exec_command
chan.exec_command(command)
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko/lib/python3.6/site-packages/paramiko/channel.py", line 72, in _check
return func(self, *args, **kwds)
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko/lib/python3.6/site-packages/paramiko/channel.py", line 257, in exec_command
self._wait_for_event()
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko/lib/python3.6/site-packages/paramiko/channel.py", line 1226, in _wait_for_event
raise e
paramiko.ssh_exception.SSHException: Channel closed.
>>>
From my side, i am see the following messages:
2019-12-14 08:52:37-0500 [SSHChannel session (0) on SSHService 'ssh-connection' on SSHTransport,0,127.0.0.1] executing command "pwd"
2019-12-14 08:52:37-0500 [-] CMD: pwd
2019-12-14 08:52:37-0500 [SSHChannel session (0) on SSHService 'ssh-connection' on SSHTransport,0,127.0.0.1] Unhandled Error
Traceback (most recent call last):
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko2/lib/python2.7/site-packages/twisted/python/log.py", line 86, in callWithContext
return context.call({ILogContext: newCtx}, func, *args, **kw)
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko2/lib/python2.7/site-packages/twisted/python/context.py", line 122, in callWithContext
return self.currentContext().callWithContext(ctx, func, *args, **kw)
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko2/lib/python2.7/site-packages/twisted/python/context.py", line 85, in callWithContext
return func(*args,**kw)
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko2/lib/python2.7/site-packages/twisted/conch/ssh/channel.py", line 162, in requestReceived
return f(data)
--- <exception caught here> ---
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko2/lib/python2.7/site-packages/twisted/conch/ssh/session.py", line 73, in request_exec
self.session.execCommand(pp, f)
File "/cygdrive/c/users/lmckiwo/Documents/dev/virtualEnv/paramiko2/MockSSH-exec_command_17/MockSSH.py", line 328, in execCommand
if args == self.commands[cmd].required_arguments[1:]:
exceptions.AttributeError: type object 'command_pwd' has no attribute 'required_arguments'
2019-12-14 08:52:37-0500 [SSHChannel session (0) on SSHService 'ssh-connection' on SSHTransport,0,127.0.0.1] remote close
2019-12-14 08:52:37-0500 [SSHChannel session (0) on SSHService 'ssh-connection' on SSHTransport,0,127.0.0.1] sending close 0
If anyone has any suggestions, please let me know. This would be a great benefit for me.
BTW, any chance this would be ported for python3?
it would be great if you can add in the support for execCommand below is the output from git diff