Merge ~mitchellaugustin/curtin/+git/curtin-1:missing-ischroot-mitigation into curtin:master

[Mitchell Augustin]

Check to ensure ischroot exists before ChrootableTarget bind mount

Some systems (such as centos7) do not include /usr/bin/ischroot,
which will cause the bind mount to it in ChrootableTarget to fail.

Adding a check for this ensures that the file exists before
mounting and aborts the mount if it does not.

Check to ensure ischroot exists before ChrootableTarget bind mount

[Dan Bungert (dbungert)]

Thanks, I think this is correct.
Do you mind adding a unittest case for that?

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:dc37aa2895114b8306afc1d446a5a7f0b4856946
https://jenkins.canonical.com/server-team/job/curtin-ci/244/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/244/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/244/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/244/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/244/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/244//rebuild

review: Approve (continuous-integration)

[Mitchell Augustin (mitchellaugustin)]

CI passed after I committed the tests, so I went ahead and squashed that commit.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:c34fe6f5117e23ccf99f794d0378b195f5c493a4
https://jenkins.canonical.com/server-team/job/curtin-ci/246/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/246/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/246/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/246/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/246/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/246//rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:935c59ff250d1b61d4e092f2fc0c06bcfd9ad991
https://jenkins.canonical.com/server-team/job/curtin-ci/247/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/247/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/247/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/247/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/247/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/247//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

Nice work!

review: Approve

diff --git a/curtin/util.py b/curtin/util.py
index 8be6d20..58ee7e1 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -784,8 +784,10 @@ class ChrootableTarget(object):
         ischroot_mount_path = paths.target_path(self.target,
                                                 '/usr/bin/ischroot')
         true_exists = os.path.isfile(true_mount_path)
-        if true_exists and do_mount(true_mount_path, ischroot_mount_path,
-                                    opts='--bind'):
+        ischroot_exists = os.path.isfile(ischroot_mount_path)
+        both_exist = true_exists and ischroot_exists
+        if both_exist and do_mount(true_mount_path, ischroot_mount_path,
+                                   opts='--bind'):
             self.umounts.append(ischroot_mount_path)
 
         return self
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 5dcef36..2dd63a7 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -642,6 +642,7 @@ class TestChrootableTargetIsChrootBehavior(CiTestCase):
         Assumptions:
         - /usr/bin/true exists inside the target. (If it does not,
         util.py does not attempt this bind mount)
+        - /usr/bin/ischroot exists inside the target.
 
         Notes:
         - This test ignores any attempts to bind-mount anything other
@@ -694,6 +695,53 @@ class TestChrootableTargetIsChrootBehavior(CiTestCase):
             self.assertEqual(self.true_content, target_ischroot_file)
 
 
+class TestChrootableTargetIsChrootBehaviorMissingFiles(CiTestCase):
+    """Test ChrootableTargets do *not* mount ischroot if ischroot or
+    true do not exist
+
+    The test checks to make sure that, in a ChrootableTarget that is
+    missing /usr/bin/ischroot or /usr/bin/true:
+        The ischroot->true bind mount is not attempted
+
+        Notes:
+        - This test ignores any attempts to bind-mount anything other
+        than files, as such mounts are out-of-scope for this test.
+        """
+
+    def setUp(self):
+        super(TestChrootableTargetIsChrootBehaviorMissingFiles, self).setUp()
+        self.target = self.tmp_dir()
+        self.truebin = os.path.join(self.target, 'usr/bin/true')
+        self.ischrootbin = os.path.join(self.target, 'usr/bin/ischroot')
+        os.makedirs(os.path.dirname(self.truebin))
+        self.add_patch('curtin.util.do_mount', 'm_do_mount')
+        # Patching subp is necessary here since subp calls are attempted
+        # in ChrootableTarget.__exit__(), which will fail since
+        # self.allowed_subp=False
+        self.add_patch('curtin.util.subp', 'm_subp')
+        self.umounts = []
+
+    def mymount(self, src, dst, opts):
+        print('mymount(src=%s dst=%s, opts=%s)' % (src, dst, opts))
+        if dst not in self.umounts:
+            self.umounts.append(dst)
+        else:
+            return False
+
+        if os.path.isfile(src):
+            util.write_file(dst, util.load_file(src))
+        else:
+            print("Ignoring non-file bind mounts for this test")
+        return True
+
+    def test_chrootable_target_does_not_bind_if_files_missing(self):
+        target_ischroot = os.path.join(self.target, 'usr/bin/ischroot')
+
+        self.m_do_mount.side_effect = self.mymount
+        with util.ChrootableTarget(self.target) as chroot:
+            self.assertFalse(target_ischroot in chroot.umounts)
+
+
 class TestChrootableTargetResolvConf(CiTestCase):
     """Test ChrootableTargets handles target /etc/resolv.conf gracefully"""