tools: fix qvm-run --pass-io --localcmd=... vmname command

qubes.VMShell service, used by qvm-run, expects the command on the first
input line. Previously, when --localcmd was used, the command wasn't
written anywhere and the local command was connected directly to
qubes.VMShell service. And the first line of its output was interpreted
as a command.

Fix this by starting the local command separately, after sending the
command to qubes.VMShell service.

While at it, unify handling shell command and service calls in the process.
vm.run_service(..., localcmd= ) isn't that useful in general case,
because for qubes.VMShell the caller first need to send the command
before starting local process. Since the qvm-run tool needs to implement
manual starting localcmd anyway, don't use localcmd= run_service's
argument at all to unify calling methods.

There is slight behavior change: previously localcmd was started only
after establishing service connection (for example only if qrexec policy
allows), now it is started in all the cases.

Fixes QubesOS/qubes-issues#4040

qubesadmin/tests/tools/qvm_run.py

@@ -17,7 +17,6 @@
 #
 # You should have received a copy of the GNU Lesser General Public License along
 # with this program; if not, see <http://www.gnu.org/licenses/>.
-
 import io
 import os
 import unittest.mock
@@ -55,7 +54,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -91,14 +89,12 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
             }),
             ('test-vm', 'qubes.VMShell', b'command; exit\n'),
             ('test-vm2', 'qubes.VMShell', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -129,7 +125,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
                 'filter_esc': True,
-                'localcmd': None,
                 'stdout': None,
                 'stderr': None,
                 'user': None,
@@ -194,7 +189,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
                 'filter_esc': True,
-                'localcmd': None,
                 'stdout': None,
                 'stderr': None,
                 'user': None,
@@ -228,7 +222,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
                 'filter_esc': self.default_filter_esc(),
-                'localcmd': None,
                 'stdout': None,
                 'stderr': None,
                 'user': None,
@@ -265,7 +258,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
                 'filter_esc': False,
-                'localcmd': None,
                 'stdout': None,
                 'stderr': None,
                 'user': None,
@@ -276,7 +268,8 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         stdout.close()
         self.assertAllCalled()
 
-    def test_005_localcmd(self):
+    @unittest.mock.patch('subprocess.Popen')
+    def test_005_localcmd(self, mock_popen):
         self.app.expected_calls[
             ('dom0', 'admin.vm.List', None, None)] = \
             b'0\x00test-vm class=AppVM state=Running\n'
@@ -286,6 +279,7 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         # self.app.expected_calls[
         #     ('test-vm', 'admin.vm.List', None, None)] = \
         #     b'0\x00test-vm class=AppVM state=Running\n'
+        mock_popen.return_value.wait.return_value = 0
         ret = qubesadmin.tools.qvm_run.main(
             ['--no-gui', '--pass-io', '--localcmd', 'local-command',
                 'test-vm', 'command'],
@@ -293,13 +287,16 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
-                'localcmd': 'local-command',
-                'stdout': None,
+                'stdout': subprocess.PIPE,
+                'stdin': subprocess.PIPE,
                 'stderr': None,
                 'user': None,
             }),
             ('test-vm', 'qubes.VMShell', b'command; exit\n')
         ])
+        mock_popen.assert_called_once_with('local-command',
+            # TODO: check if the right stdin/stdout objects are used
+            stdout=unittest.mock.ANY, stdin=unittest.mock.ANY, shell=True)
         self.assertAllCalled()
 
     def test_006_run_single_with_gui(self):
@@ -327,7 +324,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
             }),
             ('test-vm', 'qubes.WaitForSession', b'user'),
             ('test-vm', 'qubes.VMShell', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -358,7 +354,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
             }),
             ('test-vm', 'qubes.WaitForSession', b'user'),
             ('test-vm', 'service.name', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -373,7 +368,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('$dispvm', 'test.service', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -388,7 +382,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('$dispvm:test-vm', 'test.service', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -412,7 +405,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('disp123', 'test.service', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -437,7 +429,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('disp123', 'test.service', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -468,7 +459,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -511,7 +501,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('disp123', 'qubes.VMShell+WaitForSession', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -540,7 +529,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('disp123', 'qubes.VMShell', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,
@@ -566,7 +554,6 @@ class TC_00_qvm_run(qubesadmin.tests.QubesTestCase):
         self.assertEqual(ret, 0)
         self.assertEqual(self.app.service_calls, [
             ('test-vm', 'qubes.VMShell', {
-                'localcmd': None,
                 'stdout': subprocess.DEVNULL,
                 'stderr': subprocess.DEVNULL,
                 'user': None,

qubesadmin/tools/qvm_run.py

@@ -19,7 +19,6 @@
 # with this program; if not, see <http://www.gnu.org/licenses/>.
 
 ''' qvm-run tool'''
-
 import os
 import subprocess
 import sys
@@ -146,11 +145,15 @@ def main(args=None, app=None):
     if not args.passio:
         run_kwargs['stdout'] = subprocess.DEVNULL
         run_kwargs['stderr'] = subprocess.DEVNULL
+    elif args.localcmd:
+        run_kwargs['stdin'] = subprocess.PIPE
+        run_kwargs['stdout'] = subprocess.PIPE
+        run_kwargs['stderr'] = None
     else:
         # connect process output to stdout/err directly if --pass-io is given
         run_kwargs['stdout'] = None
         run_kwargs['stderr'] = None
-        if not args.localcmd and args.filter_esc:
+        if args.filter_esc:
             run_kwargs['filter_esc'] = True
 
     if isinstance(args.app, qubesadmin.app.QubesLocal) and \
@@ -204,21 +207,26 @@ def main(args=None, app=None):
                         stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
                     wait_session.communicate(vm.default_user.encode())
                 if args.service:
-                    proc = vm.run_service(args.cmd,
-                        user=args.user,
-                        localcmd=args.localcmd,
-                        **run_kwargs)
+                    service = args.cmd
                 else:
                     service = 'qubes.VMShell'
                     if args.gui and args.dispvm:
                         service += '+WaitForSession'
-                    proc = vm.run_service(service,
-                        user=args.user,
-                        localcmd=args.localcmd,
-                        **run_kwargs)
+                proc = vm.run_service(service,
+                    user=args.user,
+                    **run_kwargs)
+                if not args.service:
                     proc.stdin.write(vm.prepare_input_for_vmshell(args.cmd))
                     proc.stdin.flush()
-                if args.passio and not args.localcmd:
+                if args.localcmd:
+                    local_proc = subprocess.Popen(args.localcmd,
+                        shell=True,
+                        stdout=proc.stdin,
+                        stdin=proc.stdout)
+                    # stdin is closed below
+                    proc.stdout.close()
+                    procs.append(local_proc)
+                elif args.passio:
                     copy_proc = multiprocessing.Process(target=copy_stdin,
                         args=(proc.stdin,))
                     copy_proc.start()