Merge branch 'juerg/casd-separate-user' into 'master'

Support buildbox-casd running as separate user

See merge request BuildStream/buildstream!1737

8 files changed, 87 insertions, 13 deletions

diff --git a/src/buildstream/_cas/casdprocessmanager.py b/src/buildstream/_cas/casdprocessmanager.py
index 68bb88ef0..4c9d80209 100644
--- a/src/buildstream/_cas/casdprocessmanager.py
+++ b/src/buildstream/_cas/casdprocessmanager.py
@@ -18,8 +18,10 @@
 
 import contextlib
 import os
+import random
 import shutil
 import signal
+import stat
 import subprocess
 import tempfile
 import time
@@ -52,10 +54,7 @@ class CASDProcessManager:
     def __init__(self, path, log_dir, log_level, cache_quota, protect_session_blobs):
         self._log_dir = log_dir
 
-        # Place socket in global/user temporary directory to avoid hitting
-        # the socket path length limit.
-        self._socket_tempdir = tempfile.mkdtemp(prefix="buildstream")
-        self._socket_path = os.path.join(self._socket_tempdir, "casd.sock")
+        self._socket_path = self._make_socket_path(path)
         self._connection_string = "unix:" + self._socket_path
 
         casd_args = [utils.get_host_tool("buildbox-casd")]
@@ -80,6 +79,60 @@ class CASDProcessManager:
             with _signals.blocked([signal.SIGINT], ignore=False):
                 self.process = subprocess.Popen(casd_args, cwd=path, stdout=logfile_fp, stderr=subprocess.STDOUT)
 
+    # _make_socket_path()
+    #
+    # Create a path to the CASD socket, ensuring that we don't exceed
+    # the socket path limit.
+    #
+    # Note that we *may* exceed the path limit if the python-chosen
+    # tmpdir path is very long, though this should be /tmp.
+    #
+    # Args:
+    #     path (str): The root directory for the CAS repository.
+    #
+    # Returns:
+    #     (str) - The path to the CASD socket.
+    #
+    def _make_socket_path(self, path):
+        self._socket_tempdir = tempfile.mkdtemp(prefix="buildstream")
+        # mkdtemp will create this directory in the "most secure"
+        # way. This translates to "u+rwx,go-rwx".
+        #
+        # This is a good thing, generally, since it prevents us
+        # from leaking sensitive information to other users, but
+        # it's a problem for the workflow for userchroot, since
+        # the setuid casd binary will not share a uid with the
+        # user creating the tempdir.
+        #
+        # Instead, we chmod the directory 755, and only place a
+        # symlink to the CAS directory in here, which will allow the
+        # CASD process RWX access to a directory without leaking build
+        # information.
+        os.chmod(
+            self._socket_tempdir,
+            stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH | stat.S_IXOTH,
+        )
+
+        os.symlink(path, os.path.join(self._socket_tempdir, "cas"))
+        # FIXME: There is a potential race condition here; if multiple
+        # instances happen to create the same socket path, at least
+        # one will try to talk to the same server as us.
+        #
+        # There's no real way to avoid this from our side; we'd need
+        # buildbox-casd to tell us that it could not create a fresh
+        # socket.
+        #
+        # We could probably make this even safer by including some
+        # thread/process-specific information, but we're not really
+        # supporting this use case anyway; it's mostly here fore
+        # testing, and to help more gracefully handle the situation.
+        #
+        # Note: this uses the same random string generation principle
+        # as cpython, so this is probably a safe file name.
+        random_name = "".join([random.choice("abcdefghijklmnopqrstuvwxyz0123456789_") for _ in range(8)])
+        socket_name = "casserver-{}.sock".format(random_name)
+        return os.path.join(self._socket_tempdir, "cas", socket_name)
+
     # _rotate_and_get_next_logfile()
     #
     # Get the logfile to use for casd
diff --git a/src/buildstream/testing/_sourcetests/source_determinism.py b/src/buildstream/testing/_sourcetests/source_determinism.py
index 27664e0c2..f84d18ba2 100644
--- a/src/buildstream/testing/_sourcetests/source_determinism.py
+++ b/src/buildstream/testing/_sourcetests/source_determinism.py
@@ -23,7 +23,7 @@ import os
 import pytest
 
 from buildstream import _yaml
-from .._utils.site import HAVE_SANDBOX
+from .._utils.site import HAVE_SANDBOX, CASD_SEPARATE_USER
 from .. import create_repo
 from .. import cli  # pylint: disable=unused-import
 from .utils import kind  # pylint: disable=unused-import
@@ -101,4 +101,9 @@ def test_deterministic_source_umask(cli, tmpdir, datafiles, kind):
             os.umask(old_umask)
             cli.remove_artifact_from_cache(project, element_name)
 
-    assert get_value_for_umask(0o022) == get_value_for_umask(0o077)
+    if CASD_SEPARATE_USER:
+        # buildbox-casd running as separate user of the same group can't
+        # function in a test environment with a too restrictive umask.
+        assert get_value_for_umask(0o002) == get_value_for_umask(0o007)
+    else:
+        assert get_value_for_umask(0o022) == get_value_for_umask(0o077)
diff --git a/src/buildstream/testing/_utils/site.py b/src/buildstream/testing/_utils/site.py
index 953d21607..5507df0f7 100644
--- a/src/buildstream/testing/_utils/site.py
+++ b/src/buildstream/testing/_utils/site.py
@@ -2,6 +2,7 @@
 # so we dont have to repeat this everywhere
 #
 import os
+import stat
 import subprocess
 import sys
 import platform
@@ -65,6 +66,10 @@ try:
 except ImportError:
     HAVE_ARPY = False
 
+casd_path = utils.get_host_tool("buildbox-casd")
+CASD_SEPARATE_USER = bool(os.stat(casd_path).st_mode & stat.S_ISUID)
+del casd_path
+
 try:
     utils.get_host_tool("buildbox")
     HAVE_BUILDBOX = True
diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py
index 5009be338..b6716a29d 100644
--- a/src/buildstream/utils.py
+++ b/src/buildstream/utils.py
@@ -326,7 +326,7 @@ def safe_link(src: str, dest: str, *, result: Optional[FileListResult] = None, _
         if e.errno == errno.EEXIST and not _unlink:
             # Target exists already, unlink and try again
             safe_link(src, dest, result=result, _unlink=True)
-        elif e.errno == errno.EXDEV:
+        elif e.errno == errno.EXDEV or e.errno == errno.EPERM:
             safe_copy(src, dest)
         else:
             raise UtilError("Failed to link '{} -> {}': {}".format(src, dest, e)) from e
diff --git a/tests/frontend/buildcheckout.py b/tests/frontend/buildcheckout.py
index ffe564e81..d6a216daf 100644
--- a/tests/frontend/buildcheckout.py
+++ b/tests/frontend/buildcheckout.py
@@ -10,7 +10,7 @@ import pytest
 
 from buildstream.testing import cli  # pylint: disable=unused-import
 from buildstream.testing import create_repo
-from buildstream.testing._utils.site import IS_WINDOWS
+from buildstream.testing._utils.site import IS_WINDOWS, CASD_SEPARATE_USER
 from buildstream import _yaml
 from buildstream._exceptions import ErrorDomain, LoadErrorReason
 from buildstream import utils
@@ -35,6 +35,9 @@ def strict_args(args, strict):
     [("strict", "copies"), ("strict", "hardlinks"), ("non-strict", "copies"), ("non-strict", "hardlinks"),],
 )
 def test_build_checkout(datafiles, cli, strict, hardlinks):
+    if CASD_SEPARATE_USER and hardlinks == "hardlinks":
+        pytest.xfail("Cannot hardlink with buildbox-casd running as a separate user")
+
     project = str(datafiles)
     checkout = os.path.join(cli.directory, "checkout")
 
@@ -580,6 +583,9 @@ def test_build_checkout_nonempty(datafiles, cli, hardlinks):
 @pytest.mark.datafiles(DATA_DIR)
 @pytest.mark.parametrize("hardlinks", [("copies"), ("hardlinks")])
 def test_build_checkout_force(datafiles, cli, hardlinks):
+    if CASD_SEPARATE_USER and hardlinks == "hardlinks":
+        pytest.xfail("Cannot hardlink with buildbox-casd running as a separate user")
+
     project = str(datafiles)
     checkout = os.path.join(cli.directory, "checkout")
     filename = os.path.join(checkout, "file.txt")
diff --git a/tests/frontend/pull.py b/tests/frontend/pull.py
index fcfd447b7..3ae394fd1 100644
--- a/tests/frontend/pull.py
+++ b/tests/frontend/pull.py
@@ -414,7 +414,7 @@ def test_pull_access_rights(cli, tmpdir, datafiles):
 
         result = cli.run(
             project=project,
-            args=["artifact", "checkout", "--hardlinks", "--no-integrate", "compose-all.bst", "--directory", checkout],
+            args=["artifact", "checkout", "--no-integrate", "compose-all.bst", "--directory", checkout],
         )
         result.assert_success()
 
@@ -440,7 +440,7 @@ def test_pull_access_rights(cli, tmpdir, datafiles):
 
         result = cli.run(
             project=project,
-            args=["artifact", "checkout", "--hardlinks", "--no-integrate", "compose-all.bst", "--directory", checkout],
+            args=["artifact", "checkout", "--no-integrate", "compose-all.bst", "--directory", checkout],
         )
         result.assert_success()
 
diff --git a/tests/integration/filter.py b/tests/integration/filter.py
index f13521cc8..2fca8957c 100644
--- a/tests/integration/filter.py
+++ b/tests/integration/filter.py
@@ -29,7 +29,7 @@ def test_filter_pass_integration(datafiles, cli):
     checkout_dir = os.path.join(project, "filter")
     result = cli.run(
         project=project,
-        args=["artifact", "checkout", "--integrate", "--hardlinks", "--directory", checkout_dir, "filter/filter.bst"],
+        args=["artifact", "checkout", "--integrate", "--directory", checkout_dir, "filter/filter.bst"],
     )
     result.assert_success()
 
diff --git a/tests/integration/source-determinism.py b/tests/integration/source-determinism.py
index a349f9eeb..14559759d 100644
--- a/tests/integration/source-determinism.py
+++ b/tests/integration/source-determinism.py
@@ -6,7 +6,7 @@ import pytest
 
 from buildstream import _yaml
 from buildstream.testing import cli_integration as cli  # pylint: disable=unused-import
-from buildstream.testing._utils.site import HAVE_SANDBOX
+from buildstream.testing._utils.site import HAVE_SANDBOX, CASD_SEPARATE_USER
 
 
 DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "project")
@@ -69,4 +69,9 @@ def test_deterministic_source_local(cli, tmpdir, datafiles):
         finally:
             cli.remove_artifact_from_cache(project, element_name)
 
-    assert get_value_for_mask(0o7777) == get_value_for_mask(0o0700)
+    if CASD_SEPARATE_USER:
+        # buildbox-casd running as separate user of the same group can't
+        # read files with too restrictive permissions.
+        assert get_value_for_mask(0o7777) == get_value_for_mask(0o0770)
+    else:
+        assert get_value_for_mask(0o7777) == get_value_for_mask(0o0700)