main: set more permissive umask when creating work_dir (#9448)

* main: set more permissive umask when creating work_dir

This'll guarantee our working dir has the appropriate permissions,
even when a user has a strict umask

* update changelog

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

certbot-apache/certbot_apache/_internal/http_01.py

@@ -157,15 +157,13 @@ class ApacheHttp01(common.ChallengePerformer):
 
     def _set_up_challenges(self) -> List[KeyAuthorizationChallengeResponse]:
         if not os.path.isdir(self.challenge_dir):
-            old_umask = filesystem.umask(0o022)
-            try:
-                filesystem.makedirs(self.challenge_dir, 0o755)
-            except OSError as exception:
-                if exception.errno not in (errno.EEXIST, errno.EISDIR):
-                    raise errors.PluginError(
-                        "Couldn't create root for http-01 challenge")
-            finally:
-                filesystem.umask(old_umask)
+            with filesystem.temp_umask(0o022):
+                try:
+                    filesystem.makedirs(self.challenge_dir, 0o755)
+                except OSError as exception:
+                    if exception.errno not in (errno.EEXIST, errno.EISDIR):
+                        raise errors.PluginError(
+                            "Couldn't create root for http-01 challenge")
 
         responses = []
         for achall in self.achalls:

certbot/CHANGELOG.md

@@ -31,10 +31,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
   `certbot.tests.util.patch_display_util` as a temporary workaround.
 * Certbot's test API under `certbot.tests` now uses `unittest.mock` instead of the 3rd party `mock` library.
 
-
 ### Fixed
 
-*
+* Fixes a bug where the certbot working directory has unusably restrictive permissions on systems with stricter default umasks.
 
 More details about these changes can be found on our GitHub repo.
 

certbot/certbot/_internal/main.py

@@ -1642,7 +1642,10 @@ def make_or_verify_needed_dirs(config: configuration.NamespaceConfig) -> None:
 
     """
     util.set_up_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE, config.strict_permissions)
-    util.set_up_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, config.strict_permissions)
+
+    # Ensure the working directory has the expected mode, even under stricter umask settings
+    with filesystem.temp_umask(0o022):
+        util.set_up_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, config.strict_permissions)
 
     hook_dirs = (config.renewal_pre_hooks_dir,
                  config.renewal_deploy_hooks_dir,

certbot/certbot/_internal/plugins/webroot.py

@@ -193,8 +193,7 @@ to serve all files under specified web root ({0})."""
             # Change the permissions to be writable (GH #1389)
             # Umask is used instead of chmod to ensure the client can also
             # run as non-root (GH #1795)
-            old_umask = filesystem.umask(0o022)
-            try:
+            with filesystem.temp_umask(0o022):
                 # 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):
@@ -219,8 +218,6 @@ to serve all files under specified web root ({0})."""
                         raise errors.PluginError(
                             "Couldn't create root for {0} http-01 "
                             "challenge responses: {1}".format(name, exception))
-            finally:
-                filesystem.umask(old_umask)
 
             # On Windows, generate a local web.config file that allows IIS to serve expose
             # challenge files despite the fact they do not have a file extension.
@@ -246,13 +243,9 @@ to serve all files under specified web root ({0})."""
         logger.debug("Attempting to save validation to %s", validation_path)
 
         # Change permissions to be world-readable, owner-writable (GH #1795)
-        old_umask = filesystem.umask(0o022)
-
-        try:
+        with filesystem.temp_umask(0o022):
             with safe_open(validation_path, mode="wb", chmod=0o644) as validation_file:
                 validation_file.write(validation.encode())
-        finally:
-            filesystem.umask(old_umask)
 
         self.performed[root_path].add(achall)
         return response

certbot/certbot/compat/filesystem.py

@@ -1,6 +1,7 @@
 """Compat module to handle files security on Windows and Linux"""
 from __future__ import absolute_import
 
+from contextlib import contextmanager
 import errno
 import os  # pylint: disable=os-module-forbidden
 import stat
@@ -8,6 +9,7 @@ import sys
 from typing import Any
 from typing import Dict
 from typing import List
+from typing import Generator
 from typing import Optional
 
 try:
@@ -76,6 +78,23 @@ def umask(mask: int) -> int:
     return previous_umask
 
 
+@contextmanager
+def temp_umask(mask: int) -> Generator[None, None, None]:
+    """
+    Apply a umask temporarily, meant to be used in a `with` block. Uses the Certbot
+    implementation of umask.
+
+    :param int mask: The user file-creation mode mask to apply temporarily
+    """
+    old_umask: Optional[int] = None
+    try:
+        old_umask = umask(mask)
+        yield None
+    finally:
+        if old_umask is not None:
+            umask(old_umask)
+
+
 # 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

certbot/tests/compat/filesystem_test.py

@@ -286,6 +286,31 @@ class WindowsOpenTest(TempDirTestCase):
                 os.close(handler)
 
 
+class TempUmaskTests(test_util.TempDirTestCase):
+    """Tests for using the TempUmask class in `with` statements"""
+    def _check_umask(self):
+        old_umask = filesystem.umask(0)
+        filesystem.umask(old_umask)
+        return old_umask
+
+    def test_works_normally(self):
+        filesystem.umask(0o0022)
+        self.assertEqual(self._check_umask(), 0o0022)
+        with filesystem.temp_umask(0o0077):
+            self.assertEqual(self._check_umask(), 0o0077)
+        self.assertEqual(self._check_umask(), 0o0022)
+
+    def test_resets_umask_after_exception(self):
+        filesystem.umask(0o0022)
+        self.assertEqual(self._check_umask(), 0o0022)
+        try:
+            with filesystem.temp_umask(0o0077):
+                self.assertEqual(self._check_umask(), 0o0077)
+                raise Exception()
+        except:
+            self.assertEqual(self._check_umask(), 0o0022)
+
+
 @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security')
 class WindowsMkdirTests(test_util.TempDirTestCase):
     """Unit tests for Windows mkdir + makedirs functions in filesystem module"""