Keep the .macct of a synced image owner only

usr/lib/python3/dist-packages/linuxmusterTools/linbo/image_sync.py

@@ -29,6 +29,89 @@
 IMAGE_EXTS = {".qcow2", ".qdiff", ".cloop"}
 INCOMING_DIR_NAME = ".incoming"
 
+# The .macct file of an image holds the Samba secrets (unicodePwd,
+# supplementalCredentials) of the client the image was taken from. The
+# [linbo] rsync module serves the whole LINBO directory without login, as
+# nobody, so this file has to stay root:root 0600, like linuxmuster-linbo7
+# keeps it (rsync-post-upload.sh, linbo-torrent). The staging directory lies
+# in that module too, and is closed as a whole while an upload waits there.
+MACCT_EXT = ".macct"
+MACCT_MODE = 0o600
+STAGING_DIR_MODE = 0o700
+
+
+def _restrict(path: Path, mode: int) -> None:
+    """Set the mode of a path, and make it root's when running as root."""
+
+
+    os.chmod(path, mode)
+    if os.geteuid() == 0:
+        os.chown(path, 0, 0)
+
+
+def _staging_dir(images_dir: Path, image_name: str) -> Path:
+    """Create the staging directory of an image, closed to other users.
+
+    Both .incoming and its image subdirectory are set on every call, so a
+    directory created before, or opened again by a recursive chown, is
+    closed again.
+    """
+
+
+    incoming_dir = images_dir / INCOMING_DIR_NAME
+    staging_dir = incoming_dir / image_name
+    staging_dir.mkdir(mode=STAGING_DIR_MODE, parents=True, exist_ok=True)
+    for directory in (incoming_dir, staging_dir):
+        _restrict(directory, STAGING_DIR_MODE)
+    return staging_dir
+
+
+def _open_for_write(file_path: Path, flags: int):
+    """Open a file for writing, a .macct one created and kept 0600.
+
+    The mode is given to os.open() so a new .macct file never exists with a
+    wider one, then set again on the descriptor for a file that already
+    existed, or a umask that took the owner bits away.
+    """
+
+
+    flags |= os.O_WRONLY
+    if file_path.suffix != MACCT_EXT:
+        return os.fdopen(os.open(file_path, flags, 0o666), "wb")
+
+    fd = os.open(file_path, flags, MACCT_MODE)
+    try:
+        os.fchmod(fd, MACCT_MODE)
+        if os.geteuid() == 0:
+            os.fchown(fd, 0, 0)
+    except OSError:
+        os.close(fd)
+        raise
+    return os.fdopen(fd, "wb")
+
+
+def _copy_file(source: str | Path, target: str | Path) -> None:
+    """Copy a file like shutil.copy2(), a .macct one without widening its mode.
+
+    shutil.copy2() creates the copy with the umask default and copies the
+    mode of the source afterwards: a .macct would be readable while it is
+    written, and stay so if the source was. It is copied into a file opened
+    0600 instead, keeping the times like copy2() does. Used for the backup,
+    and by shutil.move() when the staged file lies on another filesystem.
+    """
+
+
+    source, target = Path(source), Path(target)
+    if source.suffix != MACCT_EXT:
+        shutil.copy2(str(source), str(target))
+        return
+
+    with open(source, "rb") as src, \
+            _open_for_write(target, os.O_CREAT | os.O_TRUNC) as dst:
+        shutil.copyfileobj(src, dst)
+    stat = source.stat()
+    os.utime(target, ns=(stat.st_atime_ns, stat.st_mtime_ns))
+
 
 def resolve_image_file(images_dir: Path, image_name: str, filename: str) -> Path:
     """Resolve and validate an image file path.
@@ -89,8 +172,7 @@ def receive_upload_chunk(
     image_name = name_checker.validate_linbo_image_name(image_name)
     filename = name_checker.validate_linbo_image_name(filename)
 
-    staging_dir = images_dir / INCOMING_DIR_NAME / image_name
-    staging_dir.mkdir(parents=True, exist_ok=True)
+    staging_dir = _staging_dir(images_dir, image_name)
     file_path = staging_dir / filename
 
     if offset is not None and offset > 0:
@@ -99,11 +181,12 @@ def receive_upload_chunk(
         current_size = file_path.stat().st_size
         if current_size != offset:
             raise ValueError(f"Offset mismatch: expected {current_size}, got {offset}")
-        with open(file_path, "r+b") as f:
+        with _open_for_write(file_path, 0) as f:
             f.seek(offset)
             f.write(data)
     else:
-        file_path.write_bytes(data)
+        with _open_for_write(file_path, os.O_CREAT | os.O_TRUNC) as f:
+            f.write(data)
 
     return {"received": len(data), "offset": (offset or 0) + len(data)}
 
@@ -158,7 +241,7 @@ def finalize_upload(images_dir: Path, image_name: str) -> dict:
         for f in target_dir.iterdir():
             if f.is_file():
                 try:
-                    shutil.copy2(str(f), str(backup_dir / f.name))
+                    _copy_file(f, backup_dir / f.name)
                 except OSError as e:
                     logger.warning("Backup failed for %s: %s", f.name, e)
 
@@ -169,9 +252,18 @@ def finalize_upload(images_dir: Path, image_name: str) -> dict:
     for f in staging_dir.iterdir():
         if f.is_file():
             target = target_dir / f.name
-            shutil.move(str(f), str(target))
+            # Before the move, which keeps the mode: a file staged before
+            # the staging directory was closed can still be wider.
+            if f.suffix == MACCT_EXT:
+                _restrict(f, MACCT_MODE)
+            shutil.move(str(f), str(target), copy_function=_copy_file)
             moved.append(f.name)
 
+    # A .macct left from an earlier upload or an older version is closed too.
+    for f in target_dir.iterdir():
+        if f.is_file() and f.suffix == MACCT_EXT:
+            _restrict(f, MACCT_MODE)
+
     try:
         staging_dir.rmdir()
     except OSError:

usr/lib/python3/dist-packages/linuxmusterTools/linbo/pytests/test_image_sync_permissions.py

@@ -0,0 +1,234 @@
+"""
+Tests for the permissions of the files the image serving helpers write.
+
+The .macct file of an image holds the Samba machine account secrets of the
+client it was taken from, and the [linbo] rsync module serves the whole
+LINBO directory without login as nobody. Every .macct the helpers stage,
+move into place or back up has to be 0600, the staging directory closed
+while an upload waits there, and the other image files left as they are.
+"""
+
+import errno
+import os
+import stat
+
+import pytest
+
+import linuxmusterTools.linbo.image_sync as image_sync_module
+from linuxmusterTools.linbo.image_sync import (
+    finalize_upload,
+    receive_upload_chunk,
+)
+
+
+IMAGE = "ubuntu22"
+MACCT = "ubuntu22.qcow2.macct"
+QCOW2 = "ubuntu22.qcow2"
+
+
+def mode(path):
+    return stat.S_IMODE(path.stat().st_mode)
+
+
+@pytest.fixture(autouse=True)
+def usual_umask():
+    """The umask a service usually runs with, which leaves files 0644."""
+
+    previous = os.umask(0o022)
+    yield
+    os.umask(previous)
+
+
+@pytest.fixture
+def staging_dir(tmp_path):
+    return tmp_path / ".incoming" / IMAGE
+
+
+def test_first_chunk_creates_the_macct_owner_only(tmp_path, staging_dir):
+    receive_upload_chunk(tmp_path, IMAGE, MACCT, b"unicodePwd:: secret")
+    assert mode(staging_dir / MACCT) == 0o600
+
+
+def test_first_chunk_creates_the_macct_without_a_later_chmod(tmp_path, staging_dir, monkeypatch):
+    # Without chmod the file keeps the mode it was created with: 0600 here
+    # means it never existed readable by others, not even for a moment.
+    monkeypatch.setattr(image_sync_module.os, "chmod", lambda *args, **kwargs: None)
+    monkeypatch.setattr(image_sync_module.os, "fchmod", lambda *args, **kwargs: None)
+    receive_upload_chunk(tmp_path, IMAGE, MACCT, b"unicodePwd:: secret")
+    assert mode(staging_dir / MACCT) == 0o600
+
+
+def test_resumed_chunk_closes_a_macct_staged_readable(tmp_path, staging_dir):
+    staging_dir.mkdir(parents=True)
+    staged = staging_dir / MACCT
+    staged.write_bytes(b"unicodePwd:: ")
+    staged.chmod(0o644)
+
+    receive_upload_chunk(tmp_path, IMAGE, MACCT, b"secret", offset=len(b"unicodePwd:: "))
+
+    assert staged.read_bytes() == b"unicodePwd:: secret"
+    assert mode(staged) == 0o600
+
+
+def test_first_chunk_replaces_a_macct_staged_readable(tmp_path, staging_dir):
+    staging_dir.mkdir(parents=True)
+    staged = staging_dir / MACCT
+    staged.write_bytes(b"old")
+    staged.chmod(0o644)
+
+    receive_upload_chunk(tmp_path, IMAGE, MACCT, b"new")
+
+    assert staged.read_bytes() == b"new"
+    assert mode(staged) == 0o600
+
+
+def test_image_chunk_keeps_the_usual_mode(tmp_path, staging_dir):
+    receive_upload_chunk(tmp_path, IMAGE, QCOW2, b"qcow2")
+    receive_upload_chunk(tmp_path, IMAGE, QCOW2, b"-more", offset=5)
+    assert (staging_dir / QCOW2).read_bytes() == b"qcow2-more"
+    assert mode(staging_dir / QCOW2) == 0o644
+
+
+def test_staging_directories_are_closed(tmp_path, staging_dir):
+    receive_upload_chunk(tmp_path, IMAGE, QCOW2, b"qcow2")
+    assert mode(tmp_path / ".incoming") == 0o700
+    assert mode(staging_dir) == 0o700
+
+
+def test_staging_directories_created_open_are_closed_again(tmp_path, staging_dir):
+    staging_dir.mkdir(parents=True)
+    (tmp_path / ".incoming").chmod(0o755)
+    staging_dir.chmod(0o755)
+
+    receive_upload_chunk(tmp_path, IMAGE, QCOW2, b"qcow2")
+
+    assert mode(tmp_path / ".incoming") == 0o700
+    assert mode(staging_dir) == 0o700
+
+
+def test_finalize_moves_the_macct_owner_only(tmp_path):
+    receive_upload_chunk(tmp_path, IMAGE, QCOW2, b"qcow2")
+    receive_upload_chunk(tmp_path, IMAGE, MACCT, b"secret")
+
+    finalize_upload(tmp_path, IMAGE)
+
+    assert (tmp_path / IMAGE / MACCT).read_bytes() == b"secret"
+    assert mode(tmp_path / IMAGE / MACCT) == 0o600
+    assert mode(tmp_path / IMAGE / QCOW2) == 0o644
+
+
+def test_finalize_closes_a_macct_staged_readable(tmp_path, staging_dir):
+    staging_dir.mkdir(parents=True)
+    (staging_dir / MACCT).write_bytes(b"secret")
+    (staging_dir / MACCT).chmod(0o644)
+
+    finalize_upload(tmp_path, IMAGE)
+
+    assert mode(tmp_path / IMAGE / MACCT) == 0o600
+
+
+def test_finalize_closes_a_staged_macct_before_moving_it(tmp_path, staging_dir, monkeypatch):
+    staging_dir.mkdir(parents=True)
+    (staging_dir / MACCT).write_bytes(b"secret")
+    (staging_dir / MACCT).chmod(0o644)
+    moved_modes = {}
+    move = image_sync_module.shutil.move
+
+    def recording_move(src, dst, **kwargs):
+        moved_modes[os.path.basename(src)] = stat.S_IMODE(os.stat(src).st_mode)
+        return move(src, dst, **kwargs)
+
+    monkeypatch.setattr(image_sync_module.shutil, "move", recording_move)
+
+    finalize_upload(tmp_path, IMAGE)
+
+    assert moved_modes == {MACCT: 0o600}
+
+
+def test_finalize_moves_the_macct_across_filesystems_owner_only(tmp_path, monkeypatch):
+    receive_upload_chunk(tmp_path, IMAGE, QCOW2, b"qcow2")
+    receive_upload_chunk(tmp_path, IMAGE, MACCT, b"secret")
+
+    # rename() fails like it does across filesystems, so shutil.move()
+    # copies. Without chmod the copy keeps the mode it was created with.
+    def rename(src, dst):
+        raise OSError(errno.EXDEV, os.strerror(errno.EXDEV))
+
+    monkeypatch.setattr(image_sync_module.os, "rename", rename)
+    monkeypatch.setattr(image_sync_module.os, "chmod", lambda *args, **kwargs: None)
+    monkeypatch.setattr(image_sync_module.os, "fchmod", lambda *args, **kwargs: None)
+
+    finalize_upload(tmp_path, IMAGE)
+
+    assert (tmp_path / IMAGE / MACCT).read_bytes() == b"secret"
+    assert mode(tmp_path / IMAGE / MACCT) == 0o600
+    assert not (tmp_path / ".incoming" / IMAGE / MACCT).exists()
+    assert (tmp_path / IMAGE / QCOW2).read_bytes() == b"qcow2"
+
+
+def test_finalize_backs_up_the_macct_owner_only(tmp_path):
+    target_dir = tmp_path / IMAGE
+    target_dir.mkdir()
+    (target_dir / QCOW2).write_bytes(b"old qcow2")
+    (target_dir / MACCT).write_bytes(b"old secret")
+    (target_dir / MACCT).chmod(0o644)
+    os.utime(target_dir / MACCT, (1_000_000_000, 1_000_000_000))
+    receive_upload_chunk(tmp_path, IMAGE, QCOW2, b"new qcow2")
+    receive_upload_chunk(tmp_path, IMAGE, MACCT, b"new secret")
+
+    result = finalize_upload(tmp_path, IMAGE)
+
+    backup_dir = tmp_path / result["backup"]
+    assert (backup_dir / MACCT).read_bytes() == b"old secret"
+    assert mode(backup_dir / MACCT) == 0o600
+    assert (backup_dir / MACCT).stat().st_mtime == 1_000_000_000
+    assert mode(backup_dir / QCOW2) == 0o644
+    assert mode(target_dir / MACCT) == 0o600
+
+
+def test_finalize_closes_a_macct_the_upload_left_in_place(tmp_path):
+    target_dir = tmp_path / IMAGE
+    target_dir.mkdir()
+    (target_dir / MACCT).write_bytes(b"secret")
+    (target_dir / MACCT).chmod(0o644)
+    receive_upload_chunk(tmp_path, IMAGE, QCOW2, b"qcow2")
+
+    finalize_upload(tmp_path, IMAGE)
+
+    assert mode(target_dir / MACCT) == 0o600
+
+
+def test_macct_and_staging_become_root_owned_when_running_as_root(tmp_path, monkeypatch):
+    owned = []
+
+    def chown(path, uid, gid):
+        owned.append((os.path.relpath(path, tmp_path), uid, gid))
+
+    def fchown(fd, uid, gid):
+        chown(os.readlink(f"/proc/self/fd/{fd}"), uid, gid)
+
+    monkeypatch.setattr(image_sync_module.os, "geteuid", lambda: 0)
+    monkeypatch.setattr(image_sync_module.os, "chown", chown)
+    monkeypatch.setattr(image_sync_module.os, "fchown", fchown)
+
+    target_dir = tmp_path / IMAGE
+    target_dir.mkdir()
+    (target_dir / QCOW2).write_bytes(b"old qcow2")
+    (target_dir / MACCT).write_bytes(b"old secret")
+    receive_upload_chunk(tmp_path, IMAGE, MACCT, b"new secret")
+
+    assert set(owned) == {
+        (".incoming", 0, 0),
+        (f".incoming/{IMAGE}", 0, 0),
+        (f".incoming/{IMAGE}/{MACCT}", 0, 0),
+    }
+
+    owned.clear()
+    result = finalize_upload(tmp_path, IMAGE)
+    backup = os.path.relpath(result["backup"], tmp_path)
+
+    assert set(owned) == {
+        (f".incoming/{IMAGE}/{MACCT}", 0, 0),
+        (f"{backup}/{MACCT}", 0, 0),
+        (f"{IMAGE}/{MACCT}", 0, 0),
+    }