Merge ~newell-jensen/maas:lp1815136 into maas:master

[Newell Jensen]

LP: #1815136 -- Raise error if user tries to supply extra parameters to Virsh power address.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1815136 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 66ff2d899a564af9d79fc3bbaa8816ca358c5163

review: Approve

[Alberto Donato (ack)]

One comment inline (change itself looks good otherwise)

review: Needs Fixing

[Newell Jensen (newell-jensen)]

Added the changes. Ended up going with query instead of fragment as this is what is actually set when there are extra remote uri parameters in the virsh url string.

>>> poweraddr = "qemu+ssh://10.0.0.2/system?command=maas_ssh"
>>> from urllib.parse import urlparse
>>> parsed = urlparse(poweraddr)
>>> parsed
ParseResult(scheme='qemu+ssh', netloc='10.0.0.2', path='/system', params='', query='command=maas_ssh', fragment='')

f3c0ab1... by Newell Jensen on 2019-03-08

Review fixes.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1815136 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: f3c0ab1e417f813654f905ee648e5716988f80b0

review: Approve

[Alberto Donato (ack)]

+1, thanks!

Sorry, I indeed meant using .query :)

review: Approve

diff --git a/src/provisioningserver/drivers/pod/tests/test_virsh.py b/src/provisioningserver/drivers/pod/tests/test_virsh.py
index 3cf64ea..feea500 100644
--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
@@ -543,15 +543,24 @@ class TestVirshSSH(MAASTestCase):
             conn.login(poweraddr=factory.make_name('poweraddr')))
         self.assertThat(mock_close, MockCalledOnceWith())
 
-    def test_login_with_poweraddr_extra_parameters(self):
+    def test_login_errors_with_poweraddr_extra_parameters(self):
+        conn = virsh.VirshSSH(timeout=0.1)
+        self.addCleanup(conn.close)
+        poweraddr = "qemu+ssh://ubuntu@10.0.0.2/system?no_verify=1"
+        conn._spawn('cat')
+        self.assertRaises(
+            virsh.VirshError, conn.login, poweraddr)
+
+    def test_login_with_poweraddr_adds_extra_parameters(self):
         conn = virsh.VirshSSH(timeout=0.1)
         self.addCleanup(conn.close)
         mock_execute = self.patch(conn, '_execute')
         mock_close = self.patch(conn, 'close')
-        poweraddr = "qemu+ssh://ubuntu@10.0.0.2/system?no_verify=1"
+        poweraddr = "qemu+ssh://ubuntu@10.0.0.2/system"
         conn._spawn('cat')
         self.assertFalse(conn.login(poweraddr=poweraddr))
-        self.assertThat(mock_execute, MockCalledOnceWith(poweraddr))
+        new_poweraddr = poweraddr + "?command=/usr/lib/maas/unverified-ssh"
+        self.assertThat(mock_execute, MockCalledOnceWith(new_poweraddr))
         self.assertThat(mock_close, MockCalledOnceWith())
 
     def test_login_with_poweraddr_no_extra_parameters(self):
diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
index e278a9d..83ff19c 100644
--- a/src/provisioningserver/drivers/pod/virsh.py
+++ b/src/provisioningserver/drivers/pod/virsh.py
@@ -13,6 +13,7 @@ import os
 import string
 from tempfile import NamedTemporaryFile
 from textwrap import dedent
+from urllib.parse import urlparse
 from uuid import uuid4
 
 from lxml import etree
@@ -351,13 +352,20 @@ class VirshSSH(pexpect.spawn):
 
     def login(self, poweraddr, password=None):
         """Starts connection to virsh."""
+        # Extra paramaeters are not allowed as this is a security
+        # hole for allowing a user to run executables.
+        parsed = urlparse(poweraddr)
+        if parsed.query:
+            raise VirshError(
+                "Supplying extra parameters to the Virsh address"
+                " is not supported.")
+
         # Append unverified-ssh command if user has not
         # supplied their own extra parameters.  See,
         # https://bugs.launchpad.net/maas/+bug/1807231
         # for more details.
-        if '?' not in poweraddr:
-            poweraddr = poweraddr + '?command=' + get_path(
-                '/usr/lib/maas/unverified-ssh')
+        poweraddr = poweraddr + '?command=' + get_path(
+            '/usr/lib/maas/unverified-ssh')
         self._execute(poweraddr)
         i = self.expect(self.PROMPTS, timeout=self.timeout)
         if i == self.I_PROMPT_SSHKEY: