Merge lp:~twom/launchpad-buildd/oci-dockerfile-location-fix into lp:launchpad-buildd

[Tom Wardill]

Dockerfile needs to be an absolute path

Passing the Dockerfile argument to the docker CLI requires an absolute path.
Join the path we have for the dockerfile with the build directory.

[Colin Watson (cjwatson)]

review: Needs Fixing

[lp:~twom/launchpad-buildd/oci-dockerfile-location-fix]

416. By Tom Wardill on 2020-03-31

Use cwd instead of absolute path

417. By ubuntu@buildd on 2020-03-31

Correct changelog

418. By Tom Wardill on 2020-03-31

Check for an escaping Dockerfile path

[Colin Watson (cjwatson)]

review: Approve

[lp:~twom/launchpad-buildd/oci-dockerfile-location-fix]

419. By Tom Wardill on 2020-03-31

Better logic, named exception

420. By Tom Wardill on 2020-03-31

Symlink test

421. By ubuntu@buildd on 2020-03-31

Sort imports

=== modified file 'debian/changelog'
--- debian/changelog	2020-03-18 14:51:35 +0000
+++ debian/changelog	2020-03-31 14:13:37 +0000
@@ -1,3 +1,9 @@
+launchpad-buildd (188) UNRELEASED; urgency=medium
+
+  * Fix cwd for OCI builds, allows Dockerfile parameter. 
+
+ -- Tom Wardill <[e-mail]>  Tue, 31 Mar 2020 10:13:58 +0000
+
 launchpad-buildd (187) xenial; urgency=medium
 
   [ Colin Watson ]

=== modified file 'lpbuildd/target/build_oci.py'
--- lpbuildd/target/build_oci.py	2020-02-26 10:52:29 +0000
+++ lpbuildd/target/build_oci.py	2020-03-31 14:13:37 +0000
@@ -25,6 +25,10 @@
 logger = logging.getLogger(__name__)
 
 
+class InvalidBuildFilePath(Exception):
+    pass
+
+
 class BuildOCI(SnapBuildProxyOperationMixin, VCSOperationMixin,
                SnapStoreOperationMixin, Operation):
 
@@ -40,6 +44,7 @@
     def __init__(self, args, parser):
         super(BuildOCI, self).__init__(args, parser)
         self.bin = os.path.dirname(sys.argv[0])
+        self.buildd_path = os.path.join("/home/buildd", self.args.name)
 
     def _add_docker_engine_proxy_settings(self):
         """Add systemd file for docker proxy settings."""
@@ -59,6 +64,14 @@
                 systemd_file.flush()
                 self.backend.copy_in(systemd_file.name, file_path)
 
+    def _check_build_file_escape(self):
+        """Check the build file path doesn't escape the build directory."""
+        build_file_path = os.path.realpath(
+            os.path.join(self.buildd_path, self.args.build_file))
+        common_path = os.path.commonprefix((build_file_path, self.buildd_path))
+        if common_path != self.buildd_path:
+            raise InvalidBuildFilePath("Invalid build file path.")
+
     def run_build_command(self, args, env=None, **kwargs):
         """Run a build command in the target.
 
@@ -71,7 +84,8 @@
         full_env["SHELL"] = "/bin/sh"
         if env:
             full_env.update(env)
-        return self.backend.run(args, env=full_env, **kwargs)
+        return self.backend.run(
+            args, cwd=self.buildd_path, env=full_env, **kwargs)
 
     def install(self):
         logger.info("Running install phase...")
@@ -106,9 +120,9 @@
                     ["--build-arg", "{}={}".format(var, self.args.proxy_url)])
         args.extend(["--tag", self.args.name])
         if self.args.build_file is not None:
+            self._check_build_file_escape()
             args.extend(["--file", self.args.build_file])
-        buildd_path = os.path.join("/home/buildd", self.args.name)
-        args.append(buildd_path)
+        args.append(self.buildd_path)
         self.run_build_command(args)
 
     def run(self):

=== modified file 'lpbuildd/target/tests/test_build_oci.py'
--- lpbuildd/target/tests/test_build_oci.py	2020-02-26 10:52:29 +0000
+++ lpbuildd/target/tests/test_build_oci.py	2020-03-31 14:13:37 +0000
@@ -25,6 +25,7 @@
     )
 
 from lpbuildd.target.build_oci import (
+    InvalidBuildFilePath,
     RETCODE_FAILURE_BUILD,
     RETCODE_FAILURE_INSTALL,
     )
@@ -83,7 +84,9 @@
         build_oci = parse_args(args=args).operation
         build_oci.run_build_command(["echo", "hello world"])
         self.assertThat(build_oci.backend.run.calls, MatchesListwise([
-            RanBuildCommand(["echo", "hello world"]),
+            RanBuildCommand(
+                ["echo", "hello world"],
+                cwd="/home/buildd/test-image"),
             ]))
 
     def test_run_build_command_env(self):
@@ -96,7 +99,10 @@
         build_oci.run_build_command(
             ["echo", "hello world"], env={"FOO": "bar baz"})
         self.assertThat(build_oci.backend.run.calls, MatchesListwise([
-            RanBuildCommand(["echo", "hello world"], FOO="bar baz"),
+            RanBuildCommand(
+                ["echo", "hello world"],
+                FOO="bar baz",
+                cwd="/home/buildd/test-image")
             ]))
 
     def test_install_bzr(self):
@@ -296,7 +302,8 @@
         self.assertThat(build_oci.backend.run.calls, MatchesListwise([
             RanBuildCommand(
                 ["docker", "build", "--no-cache", "--tag", "test-image",
-                 "/home/buildd/test-image"]),
+                 "/home/buildd/test-image"],
+                cwd="/home/buildd/test-image"),
             ]))
 
     def test_build_with_file(self):
@@ -313,7 +320,8 @@
             RanBuildCommand(
                 ["docker", "build", "--no-cache", "--tag", "test-image",
                  "--file", "build-aux/Dockerfile",
-                 "/home/buildd/test-image"]),
+                 "/home/buildd/test-image"],
+                cwd="/home/buildd/test-image"),
             ]))
 
     def test_build_proxy(self):
@@ -331,7 +339,8 @@
                 ["docker", "build", "--no-cache",
                  "--build-arg", "http_proxy=http://proxy.example:3128/",
                  "--build-arg", "https_proxy=http://proxy.example:3128/",
-                 "--tag", "test-image", "/home/buildd/test-image"]),
+                 "--tag", "test-image", "/home/buildd/test-image"],
+                cwd="/home/buildd/test-image"),
             ]))
 
     def test_run_succeeds(self):
@@ -351,7 +360,8 @@
                 cwd="/home/buildd")),
             AnyMatch(RanBuildCommand(
                 ["docker", "build", "--no-cache", "--tag", "test-image",
-                 "/home/buildd/test-image"])),
+                 "/home/buildd/test-image"],
+                cwd="/home/buildd/test-image")),
             ))
 
     def test_run_install_fails(self):
@@ -405,3 +415,39 @@
         build_oci.backend.build_path = self.useFixture(TempDir()).path
         build_oci.backend.run = FailBuild()
         self.assertEqual(RETCODE_FAILURE_BUILD, build_oci.run())
+
+    def test_build_with_invalid_file_path_parent(self):
+        args = [
+            "build-oci",
+            "--backend=fake", "--series=xenial", "--arch=amd64", "1",
+            "--branch", "lp:foo", "--build-file", "../build-aux/Dockerfile",
+            "test-image",
+            ]
+        build_oci = parse_args(args=args).operation
+        build_oci.backend.add_dir('/build/test-directory')
+        self.assertRaises(InvalidBuildFilePath, build_oci.build)
+
+    def test_build_with_invalid_file_path_absolute(self):
+        args = [
+            "build-oci",
+            "--backend=fake", "--series=xenial", "--arch=amd64", "1",
+            "--branch", "lp:foo", "--build-file", "/etc/Dockerfile",
+            "test-image",
+            ]
+        build_oci = parse_args(args=args).operation
+        build_oci.backend.add_dir('/build/test-directory')
+        self.assertRaises(InvalidBuildFilePath, build_oci.build)
+
+    def test_build_with_invalid_file_path_symlink(self):
+        args = [
+            "build-oci",
+            "--backend=fake", "--series=xenial", "--arch=amd64", "1",
+            "--branch", "lp:foo", "--build-file", "Dockerfile",
+            "test-image",
+            ]
+        build_oci = parse_args(args=args).operation
+        build_oci.buildd_path = self.useFixture(TempDir()).path
+        os.symlink(
+            '/etc/hosts',
+            os.path.join(build_oci.buildd_path, 'Dockerfile'))
+        self.assertRaises(InvalidBuildFilePath, build_oci.build)