[Windows] Security model for files permissions - STEP 3e (#7182)

This PR implements the filesystem.copy_ownership_and_apply_mode method from #6497.

This method is used in two places in Certbot, replacing os.chown, to copy the owner and group owner from a file to another one, and apply to the latter the given POSIX mode.

* Implement copy_ownership_and_apply_mode

* Update certbot/compat/os.py

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

* Remove default values

* Rewrite a comment.

* Relaunch CI

* Pass as keyword arguments

* Update certbot/compat/filesystem.py

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

* Update certbot/compat/filesystem.py

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

* Update certbot/compat/filesystem.py

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

* Make the private key permissions transfer platform specific

* Update certbot/compat/filesystem.py

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

* Rename variable

* Fix comment0

* Add unit test for copy_ownership_and_apply_mode

* Adapt coverage

* Execute unconditionally chmod with copy_ownership_and_apply_mode. Improve doc.

.codecov.yml

@@ -6,13 +6,13 @@ coverage:
         flags: linux
         # Fixed target instead of auto set by #7173, can
         # be removed when flags in Codecov are added back.
-        target: 97.7
+        target: 97.6
         threshold: 0.1
         base: auto
       windows:
         flags: windows
         # Fixed target instead of auto set by #7173, can
         # be removed when flags in Codecov are added back.
-        target: 96.9
+        target: 97.0
         threshold: 0.1
         base: auto

certbot/compat/filesystem.py

@@ -44,6 +44,39 @@ def chmod(file_path, mode):
         _apply_win_mode(file_path, mode)
 
 
+# One could ask why there is no copy_ownership() function, or even a reimplementation
+# of os.chown() that would modify the ownership of file without touching the mode itself.
+# This is because on Windows, it would require recalculating the existing DACL against
+# the new owner, since the DACL is composed of ACEs that targets a specific user, not dynamically
+# the current owner of a file. This action would be necessary to keep consistency between
+# the POSIX mode applied to the file and the current owner of this file.
+# Since copying and editing arbitrary DACL is very difficult, and since we actually know
+# the mode to apply at the time the owner of a file should change, it is easier to just
+# change the owner, then reapply the known mode, as copy_ownership_and_apply_mode() does.
+def copy_ownership_and_apply_mode(src, dst, mode, copy_user, copy_group):
+    # type: (str, str, int, bool, bool) -> None
+    """
+    Copy ownership (user and optionally group on Linux) from the source to the
+    destination, then apply given mode in compatible way for Linux and Windows.
+    This replaces the os.chown command.
+    :param str src: Path of the source file
+    :param str dst: Path of the destination file
+    :param int mode: Permission mode to apply on 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:
+        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)
+    elif copy_user:
+        # There is no group handling in Windows
+        _copy_win_ownership(src, dst)
+
+    chmod(dst, mode)
+
+
 def open(file_path, flags, mode=0o777):  # pylint: disable=redefined-builtin
     # type: (str, int, int) -> int
     """
@@ -251,6 +284,18 @@ def _analyze_mode(mode):
     }
 
 
+def _copy_win_ownership(src, dst):
+    security_src = win32security.GetFileSecurity(src, win32security.OWNER_SECURITY_INFORMATION)
+    user_src = security_src.GetSecurityDescriptorOwner()
+
+    security_dst = win32security.GetFileSecurity(dst, win32security.OWNER_SECURITY_INFORMATION)
+    # Second parameter indicates, if `False`, that the owner of the file is not provided by some
+    # default mechanism, but is explicitly set instead. This is obviously what we are doing here.
+    security_dst.SetSecurityDescriptorOwner(user_src, False)
+
+    win32security.SetFileSecurity(dst, win32security.OWNER_SECURITY_INFORMATION, security_dst)
+
+
 def _generate_windows_flags(rights_desc):
     # Some notes about how each POSIX right is interpreted.
     #

certbot/compat/misc.py

@@ -17,6 +17,19 @@ except ImportError:  # pragma: no cover
 from certbot import errors
 from certbot.compat import os
 
+
+# MASK_FOR_PRIVATE_KEY_PERMISSIONS defines what are the permissions flags to keep
+# when transferring the permissions from an old private key to a new one.
+if POSIX_MODE:
+    # On Linux, we keep read/write/execute permissions
+    # for group and read permissions for everybody.
+    MASK_FOR_PRIVATE_KEY_PERMISSIONS = stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP | stat.S_IROTH
+else:
+    # On Windows, the mode returned by os.stat is not reliable,
+    # so we do not keep any permission from the previous private key.
+    MASK_FOR_PRIVATE_KEY_PERMISSIONS = 0
+
+
 def raise_for_non_administrative_windows_rights():
     # type: () -> None
     """

certbot/compat/os.py

@@ -17,6 +17,7 @@ from os import *  # type: ignore  # pylint: disable=wildcard-import,unused-wildc
 # and so not in `from os import *`.
 import os as std_os  # pylint: disable=os-module-forbidden
 import sys as std_sys
+
 ourselves = std_sys.modules[__name__]
 for attribute in dir(std_os):
     # Check if the attribute does not already exist in our module. It could be internal attributes
@@ -51,6 +52,15 @@ def chmod(*unused_args, **unused_kwargs):
                        'Use certbot.compat.filesystem.chmod() instead.')
 
 
+# Because uid is not a concept on Windows, chown is useless. In fact, it is not even available
+# on Python for Windows. So to be consistent on both platforms for Certbot, this method is
+# always forbidden.
+def chown(*unused_args, **unused_kwargs):
+    """Method os.chown() is forbidden"""
+    raise RuntimeError('Usage of os.chown() is forbidden.'
+                       'Use certbot.compat.filesystem.copy_ownership_and_apply_mode() instead.')
+
+
 # The os.open function on Windows has the same effect as a call to os.chown concerning the file
 # modes: these modes lack the correct control over the permissions given to the file. Instead,
 # filesystem.open invokes the Windows native API `CreateFile` to ensure that permissions are

certbot/plugins/webroot.py

@@ -169,19 +169,19 @@ to serve all files under specified web root ({0})."""
             # run as non-root (GH #1795)
             old_umask = os.umask(0o022)
             try:
-                stat_path = os.stat(path)
                 # We ignore the last prefix in the next iteration,
                 # as it does not correspond to a folder path ('/' or 'C:')
                 for prefix in sorted(util.get_prefixes(self.full_roots[name])[:-1], key=len):
                     try:
-                        # This is coupled with the "umask" call above because os.mkdir's
-                        # "mode" parameter may not always work under Linux:
+                        # Set owner as parent directory if possible, apply mode for Linux/Windows.
+                        # For Linux, this is coupled with the "umask" call above because
+                        # os.mkdir's "mode" parameter may not always work:
                         # https://docs.python.org/3/library/os.html#os.mkdir
-                        filesystem.mkdir(prefix, 0o0755)
+                        filesystem.mkdir(prefix, 0o755)
                         self._created_dirs.append(prefix)
-                        # Set owner as parent directory if possible
                         try:
-                            os.chown(prefix, stat_path.st_uid, stat_path.st_gid)
+                            filesystem.copy_ownership_and_apply_mode(
+                                path, prefix, 0o755, copy_user=True, copy_group=True)
                         except (OSError, AttributeError) as exception:
                             logger.info("Unable to change owner and uid of webroot directory")
                             logger.debug("Error was: %s", exception)

certbot/plugins/webroot_test.py

@@ -142,13 +142,11 @@ class AuthenticatorTest(unittest.TestCase):
             self.assertRaises(errors.PluginError, self.auth.perform, [])
         filesystem.chmod(self.path, 0o700)
 
-    @test_util.skip_on_windows('On Windows, there is no chown.')
-    @mock.patch("certbot.plugins.webroot.os.chown")
-    def test_failed_chown(self, mock_chown):
-        mock_chown.side_effect = OSError(errno.EACCES, "msg")
+    @mock.patch("certbot.plugins.webroot.filesystem.copy_ownership_and_apply_mode")
+    def test_failed_chown(self, mock_ownership):
+        mock_ownership.side_effect = OSError(errno.EACCES, "msg")
         self.auth.perform([self.achall])  # exception caught and logged
 
-
     @test_util.patch_get_utility()
     def test_perform_new_webroot_not_in_map(self, mock_get_utility):
         new_webroot = tempfile.mkdtemp()

certbot/storage.py

@@ -18,6 +18,7 @@ from certbot import crypto_util
 from certbot import error_handler
 from certbot import errors
 from certbot import util
+from certbot.compat import misc
 from certbot.compat import os
 from certbot.compat import filesystem
 from certbot.plugins import common as plugins_common
@@ -1104,13 +1105,13 @@ class RenewableCert(object):
             with util.safe_open(target["privkey"], "wb", chmod=BASE_PRIVKEY_MODE) as f:
                 logger.debug("Writing new private key to %s.", target["privkey"])
                 f.write(new_privkey)
-            # Preserve gid and (mode & 074) from previous privkey in this lineage.
-            old_mode = stat.S_IMODE(os.stat(old_privkey).st_mode) & \
-                (stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP | \
-                 stat.S_IROTH)
+            # Preserve gid and (mode & MASK_FOR_PRIVATE_KEY_PERMISSIONS)
+            # from previous privkey in this lineage.
+            old_mode = (stat.S_IMODE(os.stat(old_privkey).st_mode) &
+                        misc.MASK_FOR_PRIVATE_KEY_PERMISSIONS)
             mode = BASE_PRIVKEY_MODE | old_mode
-            os.chown(target["privkey"], -1, os.stat(old_privkey).st_gid)
-            filesystem.chmod(target["privkey"], mode)
+            filesystem.copy_ownership_and_apply_mode(
+                old_privkey, target["privkey"], mode, copy_user=False, copy_group=True)
 
         # Save everything else
         with open(target["cert"], "wb") as f:

certbot/tests/compat/filesystem_test.py

@@ -2,6 +2,8 @@
 import errno
 import unittest
 
+import mock
+
 try:
     # pylint: disable=import-error
     import win32api
@@ -270,6 +272,52 @@ class WindowsMkdirTests(test_util.TempDirTestCase):
         self.assertEqual(original_mkdir, std_os.mkdir)
 
 
+class CopyOwnershipTest(test_util.TempDirTestCase):
+    """Tests about replacement of chown: copy_ownership_and_apply_mode"""
+    def setUp(self):
+        super(CopyOwnershipTest, self).setUp()
+        self.probe_path = _create_probe(self.tempdir)
+
+    @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security')
+    def test_windows(self):
+        system = win32security.ConvertStringSidToSid(SYSTEM_SID)
+        security = win32security.SECURITY_ATTRIBUTES().SECURITY_DESCRIPTOR
+        security.SetSecurityDescriptorOwner(system, False)
+
+        with mock.patch('win32security.GetFileSecurity') as mock_get:
+            with mock.patch('win32security.SetFileSecurity') as mock_set:
+                mock_get.return_value = security
+                filesystem.copy_ownership_and_apply_mode(
+                    'dummy', self.probe_path, 0o700, copy_user=True, copy_group=False)
+
+        self.assertEqual(mock_set.call_count, 2)
+
+        first_call = mock_set.call_args_list[0]
+        security = first_call[0][2]
+        self.assertEqual(system, security.GetSecurityDescriptorOwner())
+
+        second_call = mock_set.call_args_list[1]
+        security = second_call[0][2]
+        dacl = security.GetSecurityDescriptorDacl()
+        everybody = win32security.ConvertStringSidToSid(EVERYBODY_SID)
+        self.assertTrue(dacl.GetAceCount())
+        self.assertFalse([dacl.GetAce(index) for index in range(0, dacl.GetAceCount())
+                          if dacl.GetAce(index)[2] == everybody])
+
+    @unittest.skipUnless(POSIX_MODE, reason='Test specific to Linux security')
+    def test_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:
+                    mock_stat.return_value.st_uid = 50
+                    mock_stat.return_value.st_gid = 51
+                    filesystem.copy_ownership_and_apply_mode(
+                        'dummy', self.probe_path, 0o700, copy_user=True, copy_group=True)
+
+        mock_chown.assert_called_once_with(self.probe_path, 50, 51)
+        mock_chmod.assert_called_once_with(self.probe_path, 0o700)
+
+
 class OsReplaceTest(test_util.TempDirTestCase):
     """Test to ensure consistent behavior of rename method"""
 

certbot/tests/compat/os_test.py

@@ -7,7 +7,7 @@ from certbot.compat import os
 class OsTest(unittest.TestCase):
     """Unit tests for os module."""
     def test_forbidden_methods(self):
-        for method in ['chmod', 'open', 'mkdir', 'makedirs', 'rename', 'replace']:
+        for method in ['chmod', 'chown', 'open', 'mkdir', 'makedirs', 'rename', 'replace']:
             self.assertRaises(RuntimeError, getattr(os, method))
 
 

certbot/tests/storage_test.py

@@ -588,10 +588,9 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertTrue(misc.compare_file_modes(
             os.stat(self.test_rc.version("privkey", 4)).st_mode, 0o600))
 
-    @test_util.broken_on_windows
     @mock.patch("certbot.storage.relevant_values")
-    @mock.patch("certbot.storage.os.chown")
-    def test_save_successor_maintains_gid(self, mock_chown, mock_rv):
+    @mock.patch("certbot.storage.filesystem.copy_ownership_and_apply_mode")
+    def test_save_successor_maintains_gid(self, mock_ownership, mock_rv):
         # Mock relevant_values() to claim that all values are relevant here
         # (to avoid instantiating parser)
         mock_rv.side_effect = lambda x: x
@@ -599,9 +598,9 @@ class RenewableCertTests(BaseRenewableCertTest):
             self._write_out_kind(kind, 1)
         self.test_rc.update_all_links_to(1)
         self.test_rc.save_successor(1, b"newcert", None, b"new chain", self.config)
-        self.assertFalse(mock_chown.called)
+        self.assertFalse(mock_ownership.called)
         self.test_rc.save_successor(2, b"newcert", b"new_privkey", b"new chain", self.config)
-        self.assertTrue(mock_chown.called)
+        self.assertTrue(mock_ownership.called)
 
     @mock.patch("certbot.storage.relevant_values")
     def test_new_lineage(self, mock_rv):