image: Normalize modes again when an image is finalized

docs/user/images.md

@@ -12,6 +12,9 @@ provider vocabulary, the delta layer model, and disk composition internals) is d
 ## Restrictions
 
 File permissions in built images are normalized: 0755 for directories and executables, 0644 for files.
+A layer normalizes what it captures, and the image is normalized again when it is finalized, because
+Buck stores no mode beyond the executable bit and a layer out of the cache carries the umask of the
+host that materialized it.
 This deliberately breaks suid/sgid executables and the sticky bit: these need to be replaced with
 socket-activated services and other safer alternatives. `/tmp` and `/var/tmp` keep their `1777`, which is
 restored when the image is finalized. Nothing on a `/usr` partition ought to be a secret, and

image/finalize.py

@@ -11,7 +11,9 @@
 """
 
 import contextlib
+import os
 import shutil
+import stat
 import subprocess
 import sys
 from collections.abc import Generator, Sequence
@@ -30,6 +32,47 @@ class ImageSpec(TypedDict):
     tmpfiles: list[str]
 
 
+def normalize_mode(path: Path) -> None:
+    """Normalize one inode's permissions: 0755 for directories and executables, 0644 for files.
+
+    See docs/user/images.md. Symlinks (a meaningless mode), whiteout nodes and anything else that is
+    not a file or directory are not content, so whatever they are they are not ours to rewrite.
+    """
+    mode = path.lstat().st_mode
+    if stat.S_ISDIR(mode):
+        wanted = 0o755
+    elif stat.S_ISREG(mode):
+        wanted = 0o755 if mode & 0o111 else 0o644
+    else:
+        return
+    # A mode already right is left alone: on an overlay every chmod copies the inode up.
+    if stat.S_IMODE(mode) != wanted:
+        path.chmod(wanted)
+
+
+def normalize_modes(tree: Path, *, skip: Sequence[Path] = ()) -> None:
+    """Normalize every mode beneath `tree`, leaving the paths in `skip` and what is below them alone.
+
+    A layer normalizes what it captures, but Buck stores no mode beyond the executable bit, so a
+    directory comes back out of the cache with whatever the umask of the host materializing it
+    allows: 0o775 under a developer's 0o002, which sshd for one refuses in its privilege-separation
+    directory and CI, under 0o022, never sees. A file a terminal driver writes while finalizing, such
+    as an extension-release, carries that umask too. Here is the last point before the image is
+    written, so this is where the modes docs/user/images.md promises are made true.
+    """
+    skipped = {Path(path) for path in skip}
+    normalize_mode(tree)
+    for root, dirs, files in os.walk(tree):
+        parent = Path(root)
+        for name in [*dirs, *files]:
+            path = parent / name
+            if path in skipped:
+                if name in dirs:
+                    dirs.remove(name)
+                continue
+            normalize_mode(path)
+
+
 # The directories whose whole point is a mode, so a layer cannot carry them: sticky and
 # world-writable, which is exactly what Buck cannot store and the tar filter strips.
 _WORLD_WRITABLE = ("tmp", "var/tmp")
@@ -60,6 +103,9 @@ def image(
     with rootfs.rootfs(
         "/buildroot", lowers=spec["lower"] if lowers is None else lowers, binds=binds
     ) as tree:
+        # The mounted stack carries the modes its layers were materialized with, not the image's.
+        # A bind is another artifact mounted in for the driver, and not this image's to touch.
+        normalize_modes(tree, skip=[tree / str(target).lstrip("/") for _, target in binds or ()])
         # Before the snippets, so an image that states a mode of its own still wins.
         world_writable(tree)
         apply_tmpfiles(tree, spec["tmpfiles"], program=program)

image/finalize_test.py

@@ -6,6 +6,7 @@
 buck test tine//image:test
 """
 
+import os
 import tempfile
 import unittest
 from pathlib import Path
@@ -66,6 +67,66 @@ def test_an_uncommented_locale_is_a_request(self) -> None:
         self.assertTrue(finalize.wants_locales(self.tree))
 
 
+class TestNormalizeModes(unittest.TestCase):
+    @override
+    def setUp(self) -> None:
+        self.tree = Path(self.enterContext(tempfile.TemporaryDirectory(prefix="finalize.", dir="/var/tmp")))
+
+    def test_modes(self) -> None:
+        tree = self.tree
+        tree.chmod(0o700)
+        (tree / "sub").mkdir()
+        (tree / "sub").chmod(0o1777)
+        (tree / "sub/file").write_text("file")
+        (tree / "sub/file").chmod(0o664)
+        (tree / "sub/secret").write_text("secret")
+        (tree / "sub/secret").chmod(0o600)
+        (tree / "sub/script").write_text("script")
+        (tree / "sub/script").chmod(0o777)
+        (tree / "sub/mount").write_text("mount")
+        (tree / "sub/mount").chmod(0o4755)
+        (tree / "link").symlink_to("sub/secret")
+        os.mkfifo(tree / "fifo", 0o600)
+
+        finalize.normalize_modes(tree)
+
+        self.assertEqual(tree.stat().st_mode & 0o7777, 0o755)
+        self.assertEqual((tree / "sub").stat().st_mode & 0o7777, 0o755)
+        self.assertEqual((tree / "sub/file").stat().st_mode & 0o7777, 0o644)
+        self.assertEqual((tree / "sub/secret").stat().st_mode & 0o7777, 0o644)
+        self.assertEqual((tree / "sub/script").stat().st_mode & 0o7777, 0o755)
+        self.assertEqual((tree / "sub/mount").stat().st_mode & 0o7777, 0o755)
+        self.assertTrue((tree / "link").is_symlink())
+        # not content: whatever it is, it is not ours to rewrite
+        self.assertEqual((tree / "fifo").lstat().st_mode & 0o7777, 0o600)
+
+    def test_the_directories_a_materialized_layer_comes_back_with(self) -> None:
+        # Buck keeps no directory mode, so a layer out of the cache has the materializing host's umask.
+        for name in ("usr", "usr/share", "usr/share/empty.sshd"):
+            (self.tree / name).mkdir(mode=0o775)
+        finalize.normalize_modes(self.tree)
+        for name in ("usr", "usr/share", "usr/share/empty.sshd"):
+            self.assertEqual((self.tree / name).stat().st_mode & 0o7777, 0o755)
+
+    def test_leaves_a_skipped_subtree_alone(self) -> None:
+        (self.tree / "run/tine/repart").mkdir(parents=True, mode=0o775)
+        (self.tree / "run/tine/repart/0.raw").write_text("blocks")
+        (self.tree / "run/tine/repart/0.raw").chmod(0o664)
+        (self.tree / "usr").mkdir(mode=0o775)
+        finalize.normalize_modes(self.tree, skip=[self.tree / "run/tine/repart/0.raw"])
+        self.assertEqual((self.tree / "run/tine/repart/0.raw").stat().st_mode & 0o7777, 0o664)
+        self.assertEqual((self.tree / "run/tine/repart").stat().st_mode & 0o7777, 0o755)
+        self.assertEqual((self.tree / "usr").stat().st_mode & 0o7777, 0o755)
+
+    def test_the_temporary_directories_get_their_mode_back_afterwards(self) -> None:
+        # The order image() applies them in: normalization first, the sticky bit restored after.
+        (self.tree / "tmp").mkdir(mode=0o1777)
+        finalize.normalize_modes(self.tree)
+        self.assertEqual((self.tree / "tmp").stat().st_mode & 0o7777, 0o755)
+        finalize.world_writable(self.tree)
+        self.assertEqual((self.tree / "tmp").stat().st_mode & 0o7777, 0o1777)
+
+
 class TestWorldWritable(unittest.TestCase):
     @override
     def setUp(self) -> None:

image/layer.py

@@ -9,7 +9,6 @@
 import os
 import re
 import shutil
-import stat
 import subprocess
 import sys
 import tempfile
@@ -142,25 +141,6 @@ def _remove_glob(tree: Path, value: str) -> None:
         util.remove_path(path)
 
 
-def _normalize_mode(path: Path) -> None:
-    """Normalize file permissions for path.
-
-    See docs/user/images.md.
-    """
-    mode = path.lstat().st_mode
-    if stat.S_ISDIR(mode):
-        path.chmod(0o755)
-    elif stat.S_ISREG(mode):
-        path.chmod(0o755 if mode & 0o111 else 0o644)
-    # ignore symlinks (meaningless mode), whiteout nodes, and non-files
-
-
-def _normalize_modes(tree: Path) -> None:
-    _normalize_mode(tree)
-    for path in tree.rglob("*"):
-        _normalize_mode(path)
-
-
 def _apply_filesystem(operation: list[object]) -> None:
     match operation:
         case ["mkdir", str(path)]:
@@ -271,7 +251,7 @@ def main(argv: list[str] | None = None) -> None:
             _install(install, target, Path(scratch))
         for operation in operations:
             _apply(operation, target)
-    _normalize_modes(out)
+    finalize.normalize_modes(out)
     print(f"image: applied {len(operations)} ops over {len(lower)} lower(s) -> {out}", file=sys.stderr)
 
 

image/layer_test.py

@@ -6,7 +6,6 @@
 buck test tine//image:test
 """
 
-import os
 import tempfile
 import unittest
 from pathlib import Path
@@ -66,34 +65,3 @@ def test_pattern_must_be_an_absolute_image_path(self) -> None:
         for pattern in ("relative/*", "/safe/../outside"):
             with self.subTest(pattern=pattern), self.assertRaises(SystemExit):
                 layer._remove_glob(Path("/"), pattern)
-
-
-class TestNormalization(unittest.TestCase):
-    def test_modes(self) -> None:
-        with tempfile.TemporaryDirectory(prefix="layer-test.", dir="/var/tmp") as scratch:
-            tree = Path(scratch)
-            tree.chmod(0o700)
-            (tree / "sub").mkdir()
-            (tree / "sub").chmod(0o1777)
-            (tree / "sub/file").write_text("file")
-            (tree / "sub/file").chmod(0o664)
-            (tree / "sub/secret").write_text("secret")
-            (tree / "sub/secret").chmod(0o600)
-            (tree / "sub/script").write_text("script")
-            (tree / "sub/script").chmod(0o777)
-            (tree / "sub/mount").write_text("mount")
-            (tree / "sub/mount").chmod(0o4755)
-            (tree / "link").symlink_to("sub/secret")
-            os.mkfifo(tree / "fifo", 0o600)
-
-            layer._normalize_modes(tree)
-
-            self.assertEqual(tree.stat().st_mode & 0o7777, 0o755)
-            self.assertEqual((tree / "sub").stat().st_mode & 0o7777, 0o755)
-            self.assertEqual((tree / "sub/file").stat().st_mode & 0o7777, 0o644)
-            self.assertEqual((tree / "sub/secret").stat().st_mode & 0o7777, 0o644)
-            self.assertEqual((tree / "sub/script").stat().st_mode & 0o7777, 0o755)
-            self.assertEqual((tree / "sub/mount").stat().st_mode & 0o7777, 0o755)
-            self.assertTrue((tree / "link").is_symlink())
-            # not content: whatever it is, it is not ours to rewrite
-            self.assertEqual((tree / "fifo").lstat().st_mode & 0o7777, 0o600)

image_format/sysext.py

@@ -115,7 +115,11 @@ def main(argv: list[str] | None = None) -> None:
         release_dir = tree / "usr/lib/extension-release.d"
         release_dir.mkdir(parents=True, exist_ok=True)
         content = "".join(f"{key}={value}\n" for key, value in release.items())
-        (release_dir / f"extension-release.{spec['name']}").write_text(content)
+        release_file = release_dir / f"extension-release.{spec['name']}"
+        release_file.write_text(content)
+        # Written after the mounted stack was normalized, so these carry this process's umask.
+        for created in (tree / "opt", release_dir.parent, release_dir, release_file):
+            finalize.normalize_mode(created)
 
         # Here rather than beside the DDI as an action of its own: this is the tree repart is about
         # to copy, edits and all, and nothing outside this driver has it.