Platform-agnostic method to copy owner and permissions between files (#7968)

Related to #7649 since @joohoi needs a method to copy owner and permissions together from a source file to a destination file.

This PR creates the method `copy_ownership_and_mode()` in `certbot.compat.filesystem` module to achieve this goal. Its behavior is consistent across Linux and Windows in respect to the security model that have been defined for Certbot on Windows.

The method behaves globally the same than `copy_ownership_and_apply_mode`, but this time the permissions are extracted from the source file. For Windows it means that the DACL is copied from the source to the destination with the same content.

* Create copy_ownership_and_mode to copy both owner and mode from src to dst

* Update certbot/tests/compat/filesystem_test.py

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

* Fix docstring

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

certbot/certbot/compat/filesystem.py

@@ -78,6 +78,35 @@ def copy_ownership_and_apply_mode(src, dst, mode, copy_user, copy_group):
     chmod(dst, mode)
 
 
+# Quite similar to copy_ownership_and_apply_mode, but this time the DACL is copied from
+# the source file on Windows. The DACL stays consistent with the dynamic rights of the
+# equivalent POSIX mode, because ownership and mode are copied altogether on the destination
+# file, so no recomputing of the DACL against the new owner is needed, as it would be
+# for a copy_ownership alone method.
+def copy_ownership_and_mode(src, dst, copy_user=True, copy_group=True):
+    # type: (str, str, bool, bool) -> None
+    """
+    Copy ownership (user and optionally group on Linux) and mode/DACL
+    from the source to the destination.
+    :param str src: Path of the source file
+    :param str dst: Path of the destination file
+    :param bool copy_user: Copy user if `True`
+    :param bool copy_group: Copy group if `True` on Linux (has no effect on Windows)
+    """
+    if POSIX_MODE:
+        # On Linux, we just delegate to chown and chmod.
+        stats = os.stat(src)
+        user_id = stats.st_uid if copy_user else -1
+        group_id = stats.st_gid if copy_group else -1
+        os.chown(dst, user_id, group_id)
+        chmod(dst, stats.st_mode)
+    else:
+        if copy_user:
+            # There is no group handling in Windows
+            _copy_win_ownership(src, dst)
+        _copy_win_mode(src, dst)
+
+
 def check_mode(file_path, mode):
     # type: (str, int) -> bool
     """
@@ -515,6 +544,9 @@ def _analyze_mode(mode):
 
 
 def _copy_win_ownership(src, dst):
+    # Resolve symbolic links
+    src = realpath(src)
+
     security_src = win32security.GetFileSecurity(src, win32security.OWNER_SECURITY_INFORMATION)
     user_src = security_src.GetSecurityDescriptorOwner()
 
@@ -526,6 +558,19 @@ def _copy_win_ownership(src, dst):
     win32security.SetFileSecurity(dst, win32security.OWNER_SECURITY_INFORMATION, security_dst)
 
 
+def _copy_win_mode(src, dst):
+    # Resolve symbolic links
+    src = realpath(src)
+
+    # Copy the DACL from src to dst.
+    security_src = win32security.GetFileSecurity(src, win32security.DACL_SECURITY_INFORMATION)
+    dacl = security_src.GetSecurityDescriptorDacl()
+
+    security_dst = win32security.GetFileSecurity(dst, win32security.DACL_SECURITY_INFORMATION)
+    security_dst.SetSecurityDescriptorDacl(1, dacl, 0)
+    win32security.SetFileSecurity(dst, win32security.DACL_SECURITY_INFORMATION, security_dst)
+
+
 def _generate_windows_flags(rights_desc):
     # Some notes about how each POSIX right is interpreted.
     #

certbot/tests/compat/filesystem_test.py

@@ -280,14 +280,14 @@ class WindowsMkdirTests(test_util.TempDirTestCase):
         self.assertEqual(original_mkdir, std_os.mkdir)
 
 
-class OwnershipTest(test_util.TempDirTestCase):
-    """Tests about copy_ownership_and_apply_mode and has_same_ownership"""
+class CopyOwnershipAndModeTest(test_util.TempDirTestCase):
+    """Tests about copy_ownership_and_apply_mode, copy_ownership_and_mode and has_same_ownership"""
     def setUp(self):
-        super(OwnershipTest, self).setUp()
+        super(CopyOwnershipAndModeTest, self).setUp()
         self.probe_path = _create_probe(self.tempdir)
 
     @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security')
-    def test_copy_ownership_windows(self):
+    def test_copy_ownership_and_apply_mode_windows(self):
         system = win32security.ConvertStringSidToSid(SYSTEM_SID)
         security = win32security.SECURITY_ATTRIBUTES().SECURITY_DESCRIPTOR
         security.SetSecurityDescriptorOwner(system, False)
@@ -313,7 +313,7 @@ class OwnershipTest(test_util.TempDirTestCase):
                           if dacl.GetAce(index)[2] == everybody])
 
     @unittest.skipUnless(POSIX_MODE, reason='Test specific to Linux security')
-    def test_copy_ownership_linux(self):
+    def test_copy_ownership_and_apply_mode_linux(self):
         with mock.patch('os.chown') as mock_chown:
             with mock.patch('os.chmod') as mock_chmod:
                 with mock.patch('os.stat') as mock_stat:
@@ -334,6 +334,24 @@ class OwnershipTest(test_util.TempDirTestCase):
 
         self.assertTrue(filesystem.has_same_ownership(path1, path2))
 
+    @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security')
+    def test_copy_ownership_and_mode_windows(self):
+        src = self.probe_path
+        dst = _create_probe(self.tempdir, name='dst')
+
+        filesystem.chmod(src, 0o700)
+        self.assertTrue(filesystem.check_mode(src, 0o700))
+        self.assertTrue(filesystem.check_mode(dst, 0o744))
+
+        # Checking an actual change of owner is tricky during a unit test, since we do not know
+        # if any user exists beside the current one. So we mock _copy_win_ownership. It's behavior
+        # have been checked theoretically with test_copy_ownership_and_apply_mode_windows.
+        with mock.patch('certbot.compat.filesystem._copy_win_ownership') as mock_copy_owner:
+            filesystem.copy_ownership_and_mode(src, dst)
+
+        mock_copy_owner.assert_called_once_with(src, dst)
+        self.assertTrue(filesystem.check_mode(dst, 0o700))
+
 
 class CheckPermissionsTest(test_util.TempDirTestCase):
     """Tests relative to functions that check modes."""
@@ -537,9 +555,9 @@ def _set_owner(target, security_owner, user):
         target, win32security.OWNER_SECURITY_INFORMATION, security_owner)
 
 
-def _create_probe(tempdir):
+def _create_probe(tempdir, name='probe'):
     filesystem.chmod(tempdir, 0o744)
-    probe_path = os.path.join(tempdir, 'probe')
+    probe_path = os.path.join(tempdir, name)
     util.safe_open(probe_path, 'w', chmod=0o744).close()
     return probe_path