From f6380571f85e8a432018b1c99f5b080b209ed9b7 Mon Sep 17 00:00:00 2001 From: Tom Lehmann Date: Tue, 29 Sep 2026 09:54:21 +0200 Subject: [PATCH] Keep the .macct of a synced image owner only. An image pushed by a satellite brings its .macct, the Samba machine account secrets (unicodePwd, supplementalCredentials) of the client it was taken from. receive_upload_chunk() created it with the umask default, 0644, and finalize_upload() kept that mode through shutil.move() and shutil.copy2(). The [linbo] rsync module serves the whole LINBO directory without login, as nobody, so anyone reaching rsync could download it, from the image directory, its backup or .incoming while the upload was staged. Create a staged .macct 0600, with the mode given to open() so it is never readable in between, and set it again on a resumed chunk. Copy it into a file opened 0600 instead of using copy2(), which writes the copy with the umask default first: for the backup, and for a move that has to copy because .incoming and the image directory lie on different filesystems. Close a staged .macct before moving it, and every .macct of the image directory after. Keep .incoming and its image subdirectory 0700. Running as root, all of these become root:root, as linuxmuster-linbo7 does in rsync-post-upload.sh and linbo-torrent. Other image files keep their mode. --- .../linuxmusterTools/linbo/image_sync.py | 104 +++++++- .../pytests/test_image_sync_permissions.py | 234 ++++++++++++++++++ 2 files changed, 332 insertions(+), 6 deletions(-) create mode 100644 usr/lib/python3/dist-packages/linuxmusterTools/linbo/pytests/test_image_sync_permissions.py diff --git a/usr/lib/python3/dist-packages/linuxmusterTools/linbo/image_sync.py b/usr/lib/python3/dist-packages/linuxmusterTools/linbo/image_sync.py index ac90ad8..4e27e52 100644 --- a/usr/lib/python3/dist-packages/linuxmusterTools/linbo/image_sync.py +++ b/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: diff --git a/usr/lib/python3/dist-packages/linuxmusterTools/linbo/pytests/test_image_sync_permissions.py b/usr/lib/python3/dist-packages/linuxmusterTools/linbo/pytests/test_image_sync_permissions.py new file mode 100644 index 0000000..bb22bf2 --- /dev/null +++ b/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), + }