[master] Make file.manage_file permission, user and group atomic

changelog/65651.fixed.md

@@ -0,0 +1 @@
+Fix ``file.manage_file`` race condition where a copied file temporarily had 0600 permissions and the running process's ownership. The destination file now has the requested mode and owner applied atomically as part of the copy. Fixes ownership regression when only mode is specified and preserves prior file ownership when no user/group arguments are given.

salt/modules/file.py

@@ -7031,6 +7031,9 @@ def manage_file(
                     real_name,
                     __salt__["config.backup_mode"](backup),
                     __opts__["cachedir"],
+                    mode,
+                    user,
+                    group,
                 )
             except OSError as io_error:
                 __clean_tmp(sfn)
@@ -7091,6 +7094,9 @@ def manage_file(
                         real_name,
                         __salt__["config.backup_mode"](backup),
                         __opts__["cachedir"],
+                        mode,
+                        user,
+                        group,
                     )
                 except OSError as io_error:
                     __clean_tmp(tmp)
@@ -7141,6 +7147,9 @@ def manage_file(
                     name,
                     __salt__["config.backup_mode"](backup),
                     __opts__["cachedir"],
+                    mode,
+                    user,
+                    group,
                 )
             except OSError as io_error:
                 __clean_tmp(sfn)
@@ -7346,13 +7355,25 @@ def _set_mode_and_make_dirs(name, dir_mode, mode, user, group):
 
             # Copy into place
             salt.utils.files.copyfile(
-                tmp, name, __salt__["config.backup_mode"](backup), __opts__["cachedir"]
+                tmp,
+                name,
+                __salt__["config.backup_mode"](backup),
+                __opts__["cachedir"],
+                mode,
+                user,
+                group,
             )
             __clean_tmp(tmp)
         # Now copy the file contents if there is a source file
         elif sfn:
             salt.utils.files.copyfile(
-                sfn, name, __salt__["config.backup_mode"](backup), __opts__["cachedir"]
+                sfn,
+                name,
+                __salt__["config.backup_mode"](backup),
+                __opts__["cachedir"],
+                mode,
+                user,
+                group,
             )
             __clean_tmp(sfn)
 

salt/utils/files.py

@@ -188,7 +188,9 @@ def recursive_copy(source, dest):
             shutil.copyfile(file_path_from_source, target_path)
 
 
-def copyfile(source, dest, backup_mode="", cachedir=""):
+def copyfile(
+    source, dest, backup_mode="", cachedir="", mode=None, user=None, group=None
+):
     """
     Copy files from a source to a destination in an atomic way, and if
     specified cache the file.
@@ -210,16 +212,9 @@ def copyfile(source, dest, backup_mode="", cachedir=""):
     if backup_mode == "master" or backup_mode == "both" and bkroot:
         # TODO, backup to master
         pass
-    # Get current file stats to they can be replicated after the new file is
+    # Get current file stats so they can be replicated after the new file is
     # moved to the destination path.
     fstat = None
-    # We must check for platform availability first, or use a conditional import
-    # salt.utils.platform is available, but if this function is called early, it might fail?
-    # Actually, the error says 'salt' variable is used before assignment.
-    # This means 'import salt.utils.platform' is likely missing or 'salt' is not in scope.
-    # But this file starts with 'import salt.utils.platform'.
-    # Ah, 'import salt.utils.platform' is NOT at the top level of this file in the provided context?
-    # Let me check the imports.
     if not salt.utils.platform.is_windows():
         try:
             fstat = os.stat(dest)
@@ -234,9 +229,31 @@ def copyfile(source, dest, backup_mode="", cachedir=""):
         __clean_tmp(tgt)
         raise
 
-    if fstat is not None:
-        os.chown(dest, fstat.st_uid, fstat.st_gid)
-        os.chmod(dest, fstat.st_mode)
+    if not salt.utils.platform.is_windows():
+        # Apply caller-supplied mode and ownership, falling back to the
+        # pre-copy fstat values for any attribute the caller did not specify.
+        # Guard against the literal string "keep" which manage_file may pass
+        # when keep_mode=True and cp.stat_file raises; in that case treat mode
+        # as unset and let the fstat fallback (or the subsequent check_perms
+        # call) handle it.
+        effective_mode = mode if mode not in (None, "keep") else None
+        if effective_mode is not None:
+            os.chmod(dest, int(normalize_mode(effective_mode), 8))
+        elif fstat is not None:
+            os.chmod(dest, fstat.st_mode)
+
+        # Use caller-supplied user/group; for unspecified attributes fall back
+        # to the uid/gid the destination file had before the copy.
+        effective_uid = (
+            user if user is not None else (fstat.st_uid if fstat is not None else None)
+        )
+        effective_gid = (
+            group
+            if group is not None
+            else (fstat.st_gid if fstat is not None else None)
+        )
+        if effective_uid is not None or effective_gid is not None:
+            shutil.chown(dest, effective_uid, effective_gid)
     # If SELINUX is available run a restorecon on the file
     rcon = salt.utils.path.which("restorecon")
     if rcon:

tests/pytests/unit/utils/test_files.py

@@ -418,3 +418,167 @@ async def test_await_lock_raises_when_lock_path_is_directory(tmp_path):
     with pytest.raises(salt.exceptions.FileLockError, match="not a file"):
         async with salt.utils.files.await_lock(lock_fn, lock_fn=lock_fn, timeout=1):
             pass
+
+
+# ---------------------------------------------------------------------------
+# copyfile() tests — mode/user/group argument matrix
+# ---------------------------------------------------------------------------
+
+
+def _make_fstat(uid=1000, gid=1000, mode=0o644):
+    """Return a minimal os.stat_result-like mock."""
+    st = MagicMock()
+    st.st_uid = uid
+    st.st_gid = gid
+    st.st_mode = mode
+    return st
+
+
+def _copyfile_patches(fstat=None, is_windows=False, restorecon=None, dest_path=None):
+    """
+    Return a context-manager factory that stubs out the I/O-heavy parts of
+    copyfile() so we can inspect chmod / chown calls in isolation.
+
+    fstat      - if not None, returned by os.stat(dest_path); if None, os.stat
+                 raises OSError for dest_path (simulates a brand-new file).
+    is_windows - controls salt.utils.platform.is_windows()
+    restorecon - path returned by salt.utils.path.which("restorecon")
+    dest_path  - the destination path string; os.stat is only intercepted for
+                 this path (calls for other paths use the real os.stat).
+    """
+    import contextlib
+
+    _real_stat = os.stat
+
+    def _stat_side_effect(path, *args, **kwargs):
+        if dest_path is not None and str(path) == str(dest_path):
+            if fstat is not None:
+                return fstat
+            raise OSError(f"[Errno 2] No such file or directory: {path}")
+        return _real_stat(path, *args, **kwargs)
+
+    @contextlib.contextmanager
+    def _patches():
+        with patch(
+            "salt.utils.files.salt.utils.platform.is_windows", return_value=is_windows
+        ), patch("salt.utils.files.os.stat", side_effect=_stat_side_effect), patch(
+            "salt.utils.files.shutil.chown"
+        ) as mock_chown, patch(
+            "salt.utils.files.os.chmod"
+        ) as mock_chmod, patch(
+            "salt.utils.path.which", return_value=restorecon
+        ):
+            yield mock_chmod, mock_chown
+
+    return _patches
+
+
+@pytest.mark.skip_unless_on_linux
+def test_copyfile_no_args_preserves_fstat(tmp_path):
+    """When mode/user/group are all None, the destination's prior uid/gid/mode are restored."""
+    src = tmp_path / "src.txt"
+    src.write_bytes(b"hello")
+    dest = tmp_path / "dest.txt"
+    dest.write_bytes(b"old")
+
+    fstat = _make_fstat(uid=42, gid=43, mode=0o640)
+
+    with _copyfile_patches(fstat=fstat, dest_path=str(dest))() as (
+        mock_chmod,
+        mock_chown,
+    ):
+        salt.utils.files.copyfile(str(src), str(dest))
+
+    mock_chmod.assert_called_once_with(str(dest), 0o640)
+    mock_chown.assert_called_once_with(str(dest), 42, 43)
+
+
+@pytest.mark.skip_unless_on_linux
+def test_copyfile_mode_only_applies_mode_and_preserves_ownership(tmp_path):
+    """When only mode is given, it is applied and fstat uid/gid are preserved."""
+    src = tmp_path / "src.txt"
+    src.write_bytes(b"hello")
+    dest = tmp_path / "dest.txt"
+    dest.write_bytes(b"old")
+
+    fstat = _make_fstat(uid=42, gid=43, mode=0o644)
+
+    with _copyfile_patches(fstat=fstat, dest_path=str(dest))() as (
+        mock_chmod,
+        mock_chown,
+    ):
+        salt.utils.files.copyfile(str(src), str(dest), mode="755")
+
+    mock_chmod.assert_called_once_with(str(dest), 0o755)
+    mock_chown.assert_called_once_with(str(dest), 42, 43)
+
+
+@pytest.mark.skip_unless_on_linux
+def test_copyfile_mode_zero_is_applied(tmp_path):
+    """mode=0 (no permissions) must be applied — it must not be silently skipped."""
+    src = tmp_path / "src.txt"
+    src.write_bytes(b"hello")
+    dest = tmp_path / "dest.txt"
+    dest.write_bytes(b"old")
+
+    fstat = _make_fstat(uid=0, gid=0, mode=0o644)
+
+    with _copyfile_patches(fstat=fstat, dest_path=str(dest))() as (mock_chmod, _):
+        salt.utils.files.copyfile(str(src), str(dest), mode="000")
+
+    mock_chmod.assert_called_once_with(str(dest), 0o000)
+
+
+@pytest.mark.skip_unless_on_linux
+def test_copyfile_mode_keep_does_not_raise(tmp_path):
+    """mode='keep' must not raise a ValueError; fstat mode should be used instead."""
+    src = tmp_path / "src.txt"
+    src.write_bytes(b"hello")
+    dest = tmp_path / "dest.txt"
+    dest.write_bytes(b"old")
+
+    fstat = _make_fstat(uid=0, gid=0, mode=0o755)
+
+    with _copyfile_patches(fstat=fstat, dest_path=str(dest))() as (mock_chmod, _):
+        # Must not raise ValueError
+        salt.utils.files.copyfile(str(src), str(dest), mode="keep")
+
+    # fstat mode should be restored when mode="keep" cannot be resolved
+    mock_chmod.assert_called_once_with(str(dest), 0o755)
+
+
+@pytest.mark.skip_unless_on_linux
+def test_copyfile_user_and_group_applied(tmp_path):
+    """When user and group are given, they are passed to shutil.chown."""
+    src = tmp_path / "src.txt"
+    src.write_bytes(b"hello")
+    dest = tmp_path / "dest.txt"
+    dest.write_bytes(b"old")
+
+    fstat = _make_fstat(uid=0, gid=0, mode=0o644)
+
+    with _copyfile_patches(fstat=fstat, dest_path=str(dest))() as (_, mock_chown):
+        salt.utils.files.copyfile(
+            str(src), str(dest), mode="644", user="alice", group="staff"
+        )
+
+    mock_chown.assert_called_once_with(str(dest), "alice", "staff")
+
+
+@pytest.mark.skip_unless_on_linux
+def test_copyfile_new_file_no_fstat_no_args(tmp_path):
+    """For a new destination (no pre-existing file), no chown/chmod should be attempted
+    when mode/user/group are all None."""
+    src = tmp_path / "src.txt"
+    src.write_bytes(b"hello")
+    dest = tmp_path / "dest_new.txt"
+    # dest does not exist yet; fstat=None simulates the OSError from os.stat
+
+    with _copyfile_patches(fstat=None, dest_path=str(dest))() as (
+        mock_chmod,
+        mock_chown,
+    ):
+        salt.utils.files.copyfile(str(src), str(dest))
+
+    mock_chmod.assert_not_called()
+    mock_chown.assert_not_called()