Merge lp:~blake-rouse/maas/fix-1387191 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Convert TFTP request paths that contain backslashes to forward slashes.

[Christian Reis (kiko)]

r=kiko if you drop the XXX in the comment (this isn't a hack AIUI)

review: Approve

[Andres Rodriguez (andreserl)]

What's the regression potential with Trusty?
On Oct 30, 2014 3:36 PM, "Blake Rouse" <email address hidden> wrote:

> Blake Rouse has proposed merging lp:~blake-rouse/maas/fix-1387191 into
> lp:maas.
>
> Commit message:
> Convert TFTP request paths that contain backslashes to forward slashes.
>
> Requested reviews:
> MAAS Maintainers (maas-maintainers)
> Related bugs:
> Bug #1387191 in MAAS: "utopic uefi bootx64.efi not loading grubx64.efi"
> https://bugs.launchpad.net/maas/+bug/1387191
>
> For more details, see:
> https://code.launchpad.net/~blake-rouse/maas/fix-1387191/+merge/240161
> --
> https://code.launchpad.net/~blake-rouse/maas/fix-1387191/+merge/240161
> You are subscribed to branch lp:maas.
>
> === modified file
> 'src/provisioningserver/pserv_services/tests/test_tftp.py'
> --- src/provisioningserver/pserv_services/tests/test_tftp.py 2014-09-30
> 21:05:35 +0000
> +++ src/provisioningserver/pserv_services/tests/test_tftp.py 2014-10-30
> 19:35:41 +0000
> @@ -16,7 +16,7 @@
>
> from functools import partial
> import json
> -from os import path
> +import os
> from socket import (
> AF_INET,
> AF_INET6,
> @@ -140,7 +140,7 @@
>
> def get_reader(self, data):
> temp_file = self.make_file(name="example", contents=data)
> - temp_dir = path.dirname(temp_file)
> + temp_dir = os.path.dirname(temp_file)
> backend = TFTPBackend(temp_dir, "http://nowhere.example.com/")
> return backend.get_reader("example")
>
> @@ -158,6 +158,27 @@
> self.assertEqual(b"", reader.read(1))
>
> @inlineCallbacks
> + def test_get_reader_handles_backslashes_in_path(self):
> + self.patch(tftp_module, 'send_event_node_mac_address')
> + self.patch(tftp_module, 'get_remote_mac')
> +
> + data = factory.make_string().encode("ascii")
> + temp_dir = self.make_dir()
> + subdir = factory.make_name('subdir')
> + filename = factory.make_name('file')
> + os.mkdir(os.path.join(temp_dir, subdir))
> + factory.make_file(os.path.join(temp_dir, subdir), filename, data)
> +
> + path = '\\%s\\%s' % (subdir, filename)
> + backend = TFTPBackend(temp_dir, "http://nowhere.example.com/")
> + reader = yield backend.get_reader(path)
> +
> + self.addCleanup(reader.finish)
> + self.assertEqual(len(data), reader.size)
> + self.assertEqual(data, reader.read(len(data)))
> + self.assertEqual(b"", reader.read(1))
> +
> + @inlineCallbacks
> def test_get_reader_logs_node_event_with_mac_address(self):
> mac_address = factory.make_mac_address()
> self.patch_autospec(tftp_module, 'send_event_node_mac_address')
>
> === modified file 'src/provisioningserver/pserv_services/tftp.py'
> --- src/provisioningserver/pserv_services/tftp.py 2014-09-30
> 21:36:56 +0000
> +++ src/provisioningserver/pserv_services/tftp.py 2014-10-30
> 19:35:41 +0000
> @@ -209,6 +209,10 @@
> from that boot method. Otherwise the filesystem is used to service
> the response.
> """
> + # XXX blake_r 2014-10-30: It is possible for a client to request
> the
> + # file with '\'...

[Newell Jensen (newell-jensen)]

Looks good!

review: Approve

[Blake Rouse (blake-rouse)]

<slangasek> kiko, blake_r: note btw that at some later date, trusty will also be updated to include a new upstream version of shim, so any changes here should be applied for precise+trusty as well

[Blake Rouse (blake-rouse)]

but if the file uses '/' then it will work the same, like nothing has changed, which is what the current shim in trusty does.

[Andres Rodriguez (andreserl)]

Right. So that means that it will be updated sometime in the future. What
about the now? Will this cause any regressions, when say, we use trusty's?
I just want to make sure we don't introduce a regression.
On Oct 30, 2014 3:44 PM, "Blake Rouse" <email address hidden> wrote:

> <slangasek> kiko, blake_r: note btw that at some later date, trusty will
> also be updated to include a new upstream version of shim, so any changes
> here should be applied for precise+trusty as well
> --
> https://code.launchpad.net/~blake-rouse/maas/fix-1387191/+merge/240161
> You are subscribed to branch lp:maas.
>

[Christian Reis (kiko)]

Reading the code, the main risk would be if valid filenames contained forward slashes (that did not imply path separation), which I have never seen in my life and AFAIK is illegal.

=== modified file 'src/provisioningserver/pserv_services/tests/test_tftp.py'
--- src/provisioningserver/pserv_services/tests/test_tftp.py	2014-09-30 21:05:35 +0000
+++ src/provisioningserver/pserv_services/tests/test_tftp.py	2014-10-30 19:38:47 +0000
@@ -16,7 +16,7 @@
 
 from functools import partial
 import json
-from os import path
+import os
 from socket import (
     AF_INET,
     AF_INET6,
@@ -140,7 +140,7 @@
 
     def get_reader(self, data):
         temp_file = self.make_file(name="example", contents=data)
-        temp_dir = path.dirname(temp_file)
+        temp_dir = os.path.dirname(temp_file)
         backend = TFTPBackend(temp_dir, "http://nowhere.example.com/")
         return backend.get_reader("example")
 
@@ -158,6 +158,27 @@
         self.assertEqual(b"", reader.read(1))
 
     @inlineCallbacks
+    def test_get_reader_handles_backslashes_in_path(self):
+        self.patch(tftp_module, 'send_event_node_mac_address')
+        self.patch(tftp_module, 'get_remote_mac')
+
+        data = factory.make_string().encode("ascii")
+        temp_dir = self.make_dir()
+        subdir = factory.make_name('subdir')
+        filename = factory.make_name('file')
+        os.mkdir(os.path.join(temp_dir, subdir))
+        factory.make_file(os.path.join(temp_dir, subdir), filename, data)
+
+        path = '\\%s\\%s' % (subdir, filename)
+        backend = TFTPBackend(temp_dir, "http://nowhere.example.com/")
+        reader = yield backend.get_reader(path)
+
+        self.addCleanup(reader.finish)
+        self.assertEqual(len(data), reader.size)
+        self.assertEqual(data, reader.read(len(data)))
+        self.assertEqual(b"", reader.read(1))
+
+    @inlineCallbacks
     def test_get_reader_logs_node_event_with_mac_address(self):
         mac_address = factory.make_mac_address()
         self.patch_autospec(tftp_module, 'send_event_node_mac_address')

=== modified file 'src/provisioningserver/pserv_services/tftp.py'
--- src/provisioningserver/pserv_services/tftp.py	2014-09-30 21:36:56 +0000
+++ src/provisioningserver/pserv_services/tftp.py	2014-10-30 19:38:47 +0000
@@ -209,6 +209,10 @@
         from that boot method. Otherwise the filesystem is used to service
         the response.
         """
+        # It is possible for a client to request the file with '\' instead
+        # of '/', example being 'bootx64.efi'. Convert all '\' to '/' to be
+        # unix compatiable.
+        file_name = file_name.replace('\\', '/')
         mac_address = get_remote_mac()
         if mac_address is not None:
             send_event_node_mac_address(