Use isolate's /box directory for the sandbox (and other cleanups)

* remove the "secure commands" mechanism, run all commands in isolate
* remove the allow_writing_only mechanism (to be replaced with proper FS
  quotas)
* simplify the cleanup() logic, always delete sandboxes after we are
  finished using them (sandbox archive works well enough for debugging
  failures)

Author: prandla

cms/conf.py

@@ -85,7 +85,8 @@ class DatabaseConfig:
 
 @dataclass()
 class WorkerConfig:
-    keep_sandbox: bool = False
+    # TODO delete entirely??
+    pass
 
 
 @dataclass()

cms/grading/Sandbox.py

@@ -234,21 +234,22 @@ def __init__(
             # to have a number in the [0, num_workers_on_this_machine) range.
             box_id = (1 + (shard % 64)) * BOXES_PER_SHARD + box_index
 
-        # We create a directory "home" inside the outer temporary directory,
-        # that will be bind-mounted to "/tmp" inside the sandbox (some
-        # compilers need "/tmp" to exist, and this is a cheap shortcut to
-        # create it). The sandbox also runs code as a different user, and so
-        # we need to ensure that they can read and write to the directory.
-        # But we don't want everybody on the system to, which is why the
-        # outer directory exists with no read permissions.
+        self.box_id = box_id
+
+        # Tell isolate to get the sandbox ready. We do our best to cleanup
+        # after ourselves, but we might have missed something if a previous
+        # worker was interrupted in the middle of an execution, so we issue an
+        # idempotent cleanup.
+        self.cleanup()
+
+        isolate_box_dir = self.initialize_isolate()
+
         self._outer_dir = tempfile.mkdtemp(
-            dir=self.temp_dir, prefix="cms-%s-" % (self.name)
+            dir=self.temp_dir, prefix=f"cms-{self.name}-"
         )
-        self._home = os.path.join(self._outer_dir, "home")
-        self._home_dest = "/tmp"
-        os.mkdir(self._home)
+        self._home = os.path.join(isolate_box_dir, "box")
+        self._home_dest = "/box"
 
-        self.exec_name = "isolate"
         # Used for -M - the meta file ends up in the outer directory. The
         # actual filename will be <info_basename>.<execution_number>.
         self.info_basename = os.path.join(self._outer_dir, "run.meta")
@@ -259,7 +260,6 @@ def __init__(
         self.cmd_file = os.path.join(self._outer_dir, "commands.log")
 
         # Default parameters for isolate
-        self.box_id = box_id  # -b
         self.chdir = self._home_dest  # -c
         self.dirs: list[tuple[str | None, str, str | None]] = []  # -d
         self.preserve_env = False  # -e
@@ -278,10 +278,6 @@ def __init__(
 
         self.max_processes: int = 1
 
-        self.allow_writing_all()
-
-        self.add_mapped_directory(self._home, dest=self._home_dest, options="rw")
-
         # Create temporary directory on /dev/shm to prevent communication
         # between sandboxes.
         self.dirs.append((None, "/dev/shm", "tmp"))
@@ -304,13 +300,6 @@ def __init__(
         # particular, the System.Native assembly.
         self.maybe_add_mapped_directory("/etc/mono", options="noexec")
 
-        # Tell isolate to get the sandbox ready. We do our best to cleanup
-        # after ourselves, but we might have missed something if a previous
-        # worker was interrupted in the middle of an execution, so we issue an
-        # idempotent cleanup.
-        self.cleanup()
-        self.initialize_isolate()
-
     def set_multiprocess(self, multiprocess: bool):
         """Set the sandbox to (dis-)allow multiple threads and processes.
 
@@ -707,32 +696,9 @@ def cleanup(self, delete: bool = False):
         delete: if True, also delete get_root_path() and everything it
             contains.
         """
-        # The user isolate assigns within the sandbox might have created
-        # subdirectories and files therein, making the user outside the sandbox
-        # unable to delete the whole tree. If the caller asked us to delete the
-        # sandbox, we first issue a chmod within isolate to make sure that we
-        # will be able to delete everything. If not, we leave the files as they
-        # are to avoid masking possible problems the admin wanted to debug.
 
         exe = ["isolate", "--box-id=%d" % self.box_id, "--cg"]
 
-        if delete:
-            # Ignore exit status as some files may be owned by our user
-            subprocess.call(
-                exe
-                + [
-                    "--dir=%s=%s:rw" % (self._home_dest, self._home),
-                    "--run",
-                    "--",
-                    "/bin/chmod",
-                    "777",
-                    "-R",
-                    self._home_dest,
-                ],
-                stdout=subprocess.DEVNULL,
-                stderr=subprocess.STDOUT,
-            )
-
         # Tell isolate to cleanup the sandbox.
         subprocess.check_call(
             exe + ["--cleanup"], stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT
@@ -755,18 +721,24 @@ def archive(self, file_cacher: FileCacher) -> str | None:
 
         with tempfile.TemporaryFile(dir=self.temp_dir) as sandbox_archive:
             # Archive the working directory
-            content_path = self.get_root_path()
-            try:
-                with tarfile.open(fileobj=sandbox_archive, mode="w:gz") as tar_file:
-                    tar_file.add(content_path, os.path.basename(content_path))
-            except Exception:
-                logger.warning("Failed to archive sandbox", exc_info=True)
-                return None
+            metadata_path = self.get_root_path()
+            box_dir = self._home
+            with tarfile.open(fileobj=sandbox_archive, mode='w:gz') as tar_file:
+                try:
+                    # Add metadata files
+                    for x in os.listdir(metadata_path):
+                        tar_file.add(os.path.join(metadata_path, x), x)
+                    # Add the box directory
+                    tar_file.add(box_dir, "box")
+                except Exception:
+                    logger.warning(
+                        "Failed to add files to sandbox archive", exc_info=True
+                    )
 
             # Put archive to FS
             sandbox_archive.seek(0)
             return file_cacher.put_file_from_fobj(
-                sandbox_archive, "Sandbox %s" % self.get_root_path()
+                sandbox_archive, f"Sandbox {self.get_root_path()}"
             )
 
     def add_mapped_directory(
@@ -800,55 +772,6 @@ def maybe_add_mapped_directory(
             src, dest, options, ignore_if_not_existing=True
         )
 
-    def allow_writing_all(self):
-        """Set permissions in such a way that any operation is allowed."""
-        os.chmod(self._home, 0o777)
-        for filename in os.listdir(self._home):
-            os.chmod(os.path.join(self._home, filename), 0o777)
-
-    def allow_writing_none(self):
-        """Set permissions in such a way that the user cannot write anything."""
-        os.chmod(self._home, 0o755)
-        for filename in os.listdir(self._home):
-            os.chmod(os.path.join(self._home, filename), 0o755)
-
-    def allow_writing_only(self, inner_paths: list[str]):
-        """Set permissions in so that the user can write only some paths.
-
-        By default the user can only write to the home directory. This
-        method further restricts permissions so that it can only write
-        to some files inside the home directory.
-
-        inner_paths: the only paths that the user is allowed to
-            write to; they should be "inner" paths (from the perspective
-            of the sandboxed process, not of the host system); they can
-            be absolute or relative (in which case they are interpreted
-            relative to the home directory); paths that point to a file
-            outside the home directory are ignored.
-
-        """
-        outer_paths = []
-        for inner_path in inner_paths:
-            abs_inner_path = os.path.realpath(os.path.join(self._home_dest, inner_path))
-            # If an inner path is absolute (e.g., /fifo0/u0_to_m) then
-            # it may be outside home and we should ignore it.
-            if os.path.commonpath([abs_inner_path, self._home_dest]) != self._home_dest:
-                continue
-            rel_inner_path = os.path.relpath(abs_inner_path, self._home_dest)
-            outer_path = os.path.join(self._home, rel_inner_path)
-            outer_paths.append(outer_path)
-
-        # If one of the specified file do not exists, we touch it to
-        # assign the correct permissions.
-        for path in outer_paths:
-            if not os.path.exists(path):
-                open(path, "wb").close()
-
-        # Close everything, then open only the specified.
-        self.allow_writing_none()
-        for path in outer_paths:
-            os.chmod(path, 0o722)
-
     def build_box_options(self) -> list[str]:
         """Translate the options defined in the instance to a string
         that can be postponed to isolate as an arguments list.
@@ -980,12 +903,10 @@ def _popen(
             "Executing program in sandbox with command: `%s'.",
             pretty_print_cmdline(args),
         )
-        # Temporarily allow writing new files.
-        prev_permissions = stat.S_IMODE(os.stat(self._home).st_mode)
-        os.chmod(self._home, 0o770)
-        with open(self.cmd_file, "at", encoding="utf-8") as commands:
-            commands.write("%s\n" % (pretty_print_cmdline(args)))
-        os.chmod(self._home, prev_permissions)
+
+        with open(self.cmd_file, "a") as commands:
+            commands.write(pretty_print_cmdline(args) + "\n")
+
         try:
             p = subprocess.Popen(
                 args, stdin=stdin, stdout=stdout, stderr=stderr, close_fds=close_fds
@@ -1000,10 +921,12 @@ def _popen(
 
         return p
 
-    def initialize_isolate(self):
+    def initialize_isolate(self) -> str:
         """Initialize isolate's box."""
         init_cmd = ["isolate", "--box-id=%d" % self.box_id, "--cg", "--init"]
         try:
-            subprocess.check_call(init_cmd)
+            return subprocess.run(
+                init_cmd, check=True, capture_output=True, encoding="utf-8"
+            ).stdout.strip()
         except subprocess.CalledProcessError as e:
             raise SandboxInterfaceException("Failed to initialize sandbox") from e

cms/grading/steps/evaluation.py

@@ -212,7 +212,6 @@ def evaluation_step_before_run(
     for name in [sandbox.stderr_file, sandbox.stdout_file]:
         if name is not None:
             writable_files.append(name)
-    sandbox.allow_writing_only(writable_files)
 
     sandbox.set_multiprocess(multiprocess)
 

cms/grading/tasktypes/Communication.py

@@ -444,6 +444,6 @@ def evaluate(self, job: EvaluationJob, file_cacher: FileCacher):
         delete_sandbox(sandbox_mgr, job, file_cacher)
         for s in sandbox_user:
             delete_sandbox(s, job, file_cacher)
-        if job.success and not config.worker.keep_sandbox and not job.keep_sandbox:
+        if job.success and not job.keep_sandbox:
             for d in fifo_dir:
                 rmtree(d)

cms/grading/tasktypes/util.py

@@ -84,19 +84,16 @@ def delete_sandbox(sandbox: Sandbox, job: Job, file_cacher: FileCacher, success:
         success = job.success
 
     # Archive the sandbox if required
-    if job.archive_sandbox:
+    if job.archive_sandbox or not success:
         sandbox_digest = sandbox.archive(file_cacher)
         if sandbox_digest is not None:
             job.sandbox_digests[sandbox.get_root_path()] = sandbox_digest
 
-    # If the job was not successful, we keep the sandbox around.
-    if not success:
-        logger.warning("Sandbox %s kept around because job did not succeed.",
-                       sandbox.get_root_path())
+        if not success:
+            logger.warning(f"Job did not succeed! Sandbox digest: {sandbox_digest}")
 
-    delete = success and not config.worker.keep_sandbox and not job.keep_sandbox
     try:
-        sandbox.cleanup(delete=delete)
+        sandbox.cleanup(delete=True)
     except OSError:
         err_msg = "Couldn't delete sandbox."
         logger.warning(err_msg, exc_info=True)

cmstestsuite/unit_tests/grading/steps/fakesandbox.py

@@ -22,8 +22,10 @@
 import os
 from collections import deque
 from io import BytesIO, StringIO
+import tempfile
 
 from cms.grading.Sandbox import Sandbox
+from cms.util import rmtree
 
 
 class FakeSandbox(Sandbox):
@@ -139,7 +141,10 @@ def execute_without_std(self, command, wait=False):
         return data["success"]
 
     def initialize_isolate(self):
-        pass
+        tmpd = tempfile.mkdtemp(dir=self.temp_dir, prefix="cms-fake-sandbox-")
+        os.mkdir(tmpd+"/box")
+        return tmpd
 
     def cleanup(self, delete=False):
-        pass
+        if hasattr(self, '_outer_dir'):
+            rmtree(self._outer_dir)

cmstestsuite/unit_tests/grading/tasktypes/BatchTest.py

@@ -225,7 +225,7 @@ def test_alone_sandbox_failure(self):
         self.assertResultsInJob(job)
         sandbox.get_file_to_storage.assert_not_called()
         # We preserve the sandbox to let admins check the problem.
-        sandbox.cleanup.assert_called_once_with(delete=False)
+        sandbox.archive.assert_called_once()
 
     def test_grader_success(self):
         # We sprinkle in also a header, that should be copied, but not the
@@ -412,7 +412,7 @@ def test_stdio_diff_evaluation_step_sandbox_failure_(self):
         self.assertResultsInJob(job)
         # eval_output should not have been called, and the sandbox not deleted.
         self.eval_output.assert_not_called()
-        sandbox.cleanup.assert_called_once_with(delete=False)
+        sandbox.archive.assert_called_once()
 
     def test_stdio_diff_eval_output_failure_(self):
         tt, job = self.prepare(["alone", ["", ""], "diff"], {"foo": EXE_FOO})
@@ -424,7 +424,7 @@ def test_stdio_diff_eval_output_failure_(self):
         self.assertResultsInJob(job)
         # Even if the error is in the eval_output sandbox, we keep also the one
         # for evaluation_step to allow debugging.
-        sandbox.cleanup.assert_called_once_with(delete=False)
+        sandbox.archive.assert_called_once()
 
     def test_stdio_diff_get_output_success(self):
         tt, job = self.prepare(["alone", ["", ""], "diff"], {"foo": EXE_FOO})

cmstestsuite/unit_tests/grading/tasktypes/CommunicationTest.py

@@ -203,7 +203,7 @@ def test_one_file_sandbox_failure(self):
         self.assertResultsInJob(job, False, None, None, None)
         sandbox.get_file_to_storage.assert_not_called()
         # We preserve the sandbox to let admins check the problem.
-        sandbox.cleanup.assert_called_once_with(delete=False)
+        sandbox.archive.assert_called_once()
 
     def test_many_files_success(self):
         tt, job = self.prepare(
@@ -469,8 +469,8 @@ def test_single_process_manager_failure(self):
         tt.evaluate(job, self.file_cacher)
 
         self.assertResultsInJob(job, False, None, None, None)
-        sandbox_mgr.cleanup.assert_called_once_with(delete=False)
-        sandbox_usr.cleanup.assert_called_once_with(delete=False)
+        sandbox_mgr.archive.assert_called_once()
+        sandbox_usr.archive.assert_called_once()
 
     def test_single_process_manager_sandbox_failure(self):
         # Manager sandbox had problems, it's not the user's fault.
@@ -487,8 +487,8 @@ def test_single_process_manager_sandbox_failure(self):
         tt.evaluate(job, self.file_cacher)
 
         self.assertResultsInJob(job, False, None, None, None)
-        sandbox_mgr.cleanup.assert_called_once_with(delete=False)
-        sandbox_usr.cleanup.assert_called_once_with(delete=False)
+        sandbox_mgr.archive.assert_called_once()
+        sandbox_usr.archive.assert_called_once()
 
     def test_single_process_manager_and_user_failure(self):
         # Manager had problems, it's not the user's fault even if also their
@@ -506,8 +506,8 @@ def test_single_process_manager_and_user_failure(self):
         tt.evaluate(job, self.file_cacher)
 
         self.assertResultsInJob(job, False, None, None, None)
-        sandbox_mgr.cleanup.assert_called_once_with(delete=False)
-        sandbox_usr.cleanup.assert_called_once_with(delete=False)
+        sandbox_mgr.archive.assert_called_once()
+        sandbox_usr.archive.assert_called_once()
 
     def test_single_process_user_sandbox_failure(self):
         # User sandbox had problems, it's not the user's fault.
@@ -524,8 +524,8 @@ def test_single_process_user_sandbox_failure(self):
         tt.evaluate(job, self.file_cacher)
 
         self.assertResultsInJob(job, False, None, None, None)
-        sandbox_mgr.cleanup.assert_called_once_with(delete=False)
-        sandbox_usr.cleanup.assert_called_once_with(delete=False)
+        sandbox_mgr.archive.assert_called_once()
+        sandbox_usr.archive.assert_called_once()
 
     def test_single_process_user_failure(self):
         # User program had problems, it's the user's fault.

cmstestsuite/unit_tests/grading/tasktypes/tasktypetestutils.py

@@ -95,11 +95,6 @@ def setUpMocks(self, tasktype: str):
         """
         self.tasktype = tasktype
 
-        # Ensure we don't retain all sandboxes so we can verify delete().
-        patcher = patch.object(config.worker, "keep_sandbox", False)
-        self.addCleanup(patcher.stop)
-        patcher.start()
-
         # Mock the set of languages (if the task type uses it). Child classes
         # can update this dict before the test to change the set of languages
         # CMS understands.