diff --git a/certbot-apache/certbot_apache/_internal/http_01.py b/certbot-apache/certbot_apache/_internal/http_01.py index ce64d451e..5ef44fa2e 100644 --- a/certbot-apache/certbot_apache/_internal/http_01.py +++ b/certbot-apache/certbot_apache/_internal/http_01.py @@ -169,7 +169,7 @@ class ApacheHttp01(common.ChallengePerformer): def _set_up_challenges(self): if not os.path.isdir(self.challenge_dir): - old_umask = os.umask(0o022) + old_umask = filesystem.umask(0o022) try: filesystem.makedirs(self.challenge_dir, 0o755) except OSError as exception: @@ -177,7 +177,7 @@ class ApacheHttp01(common.ChallengePerformer): raise errors.PluginError( "Couldn't create root for http-01 challenge") finally: - os.umask(old_umask) + filesystem.umask(old_umask) responses = [] for achall in self.achalls: diff --git a/certbot-apache/local-oldest-requirements.txt b/certbot-apache/local-oldest-requirements.txt index e45a0f831..7a6a1e4b8 100644 --- a/certbot-apache/local-oldest-requirements.txt +++ b/certbot-apache/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.29.0 -certbot[dev]==1.1.0 \ No newline at end of file +-e certbot[dev] \ No newline at end of file diff --git a/certbot-apache/setup.py b/certbot-apache/setup.py index a00f14080..719bea004 100644 --- a/certbot-apache/setup.py +++ b/certbot-apache/setup.py @@ -12,7 +12,7 @@ version = '1.6.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.29.0', - 'certbot>=1.1.0', + 'certbot>=1.6.0.dev0', 'python-augeas', 'setuptools', 'zope.component', diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 88d128bbe..37bf98960 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -7,10 +7,15 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Added * Add minimal code to run Nginx plugin on NetBSD. +* Function `certbot.compat.filesystem.umask` is a drop-in replacement for `os.umask` + implementing umask for both UNIX and Windows systems. ### Changed * Allow session tickets to be disabled in Apache when mod_ssl is statically linked. +* Certbot behaves similarly on Windows to on UNIX systems regarding umask, and + the umask `022` is applied by default: all files/directories are not writable by anyone + other than the user running Certbot and the system/admin users. * Read acmev1 Let's Encrypt server URL from renewal config as acmev2 URL to prepare for impending acmev1 deprecation. diff --git a/certbot/certbot/_internal/plugins/webroot.py b/certbot/certbot/_internal/plugins/webroot.py index 9383ce66d..484d209d6 100644 --- a/certbot/certbot/_internal/plugins/webroot.py +++ b/certbot/certbot/_internal/plugins/webroot.py @@ -164,7 +164,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 = os.umask(0o022) + old_umask = filesystem.umask(0o022) try: # We ignore the last prefix in the next iteration, # as it does not correspond to a folder path ('/' or 'C:') @@ -191,7 +191,7 @@ to serve all files under specified web root ({0}).""" "Couldn't create root for {0} http-01 " "challenge responses: {1}".format(name, exception)) finally: - os.umask(old_umask) + filesystem.umask(old_umask) def _get_validation_path(self, root_path, achall): # pylint: no-self-use return os.path.join(root_path, achall.chall.encode("token")) @@ -204,13 +204,13 @@ 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 = os.umask(0o022) + old_umask = filesystem.umask(0o022) try: with safe_open(validation_path, mode="wb", chmod=0o644) as validation_file: validation_file.write(validation.encode()) finally: - os.umask(old_umask) + filesystem.umask(old_umask) self.performed[root_path].add(achall) return response diff --git a/certbot/certbot/compat/filesystem.py b/certbot/certbot/compat/filesystem.py index 822efabfd..081d5d915 100644 --- a/certbot/certbot/compat/filesystem.py +++ b/certbot/certbot/compat/filesystem.py @@ -21,6 +21,20 @@ else: POSIX_MODE = False +# Windows umask implementation, since Windows does not have a concept of umask by default. +# We choose 022 as initial value since it is the default one on most Linux distributions, and +# it is a decent choice to not have write permissions for group owner and everybody by default. +# We use a class here to avoid needing to define a global variable, and the potential mistakes +# that could happen with this kind of pattern. +class _WindowsUmask: + """Store the current umask to apply on Windows""" + def __init__(self): + self.mask = 0o022 + + +_WINDOWS_UMASK = _WindowsUmask() + + def chmod(file_path, mode): # type: (str, int) -> None """ @@ -43,6 +57,24 @@ def chmod(file_path, mode): _apply_win_mode(file_path, mode) +def umask(mask): + # type: (int) -> int + """ + Set the current numeric umask and return the previous umask. On Linux, the built-in umask + method is used. On Windows, our Certbot-side implementation is used. + + :param int mask: The user file-creation mode mask to apply. + :rtype: int + :return: The previous umask value. + """ + if POSIX_MODE: + return os.umask(mask) + + previous_umask = _WINDOWS_UMASK.mask + _WINDOWS_UMASK.mask = mask + return previous_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 @@ -194,7 +226,7 @@ def open(file_path, flags, mode=0o777): # pylint: disable=redefined-builtin attributes = win32security.SECURITY_ATTRIBUTES() security = attributes.SECURITY_DESCRIPTOR user = _get_current_user() - dacl = _generate_dacl(user, mode) + dacl = _generate_dacl(user, mode, _WINDOWS_UMASK.mask) # We set second parameter to 0 (`False`) to say that this security descriptor is # NOT constructed from a default mechanism, but is explicitly set by the user. # See https://docs.microsoft.com/en-us/windows/desktop/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptorowner # pylint: disable=line-too-long @@ -244,28 +276,27 @@ def makedirs(file_path, mode=0o777): :param int mode: POSIX mode to apply on leaf directory when created, Python defaults will be applied if ``None`` """ - if POSIX_MODE: - # Since Python 3.7, os.makedirs does not set the given mode to the intermediate directories - # that could be created in the process. To keep things safe and consistent on all - # Python versions, we set the umask accordingly to have all directories (intermediate and - # leaf) created with the given mode. - current_umask = os.umask(0) + current_umask = umask(0) + try: + # Since Python 3.7, os.makedirs does not set the given mode to the intermediate + # directories that could be created in the process. To keep things safe and consistent + # on all Python versions, we set the umask accordingly to have all directories + # (intermediate and leaf) created with the given mode. + umask(current_umask | 0o777 ^ mode) + + if POSIX_MODE: + return os.makedirs(file_path, mode) + + orig_mkdir_fn = os.mkdir try: - os.umask(current_umask | 0o777 ^ mode) + # As we know that os.mkdir is called internally by os.makedirs, we will swap the + # function in os module for the time of makedirs execution on Windows. + os.mkdir = mkdir # type: ignore return os.makedirs(file_path, mode) finally: - os.umask(current_umask) - - # TODO: Windows does not support umask. A specific PR (#7967) is handling this, and will need - # to add appropriate umask call for the Windows part of the logic below. - orig_mkdir_fn = os.mkdir - try: - # As we know that os.mkdir is called internally by os.makedirs, we will swap the function in - # os module for the time of makedirs execution on Windows. - os.mkdir = mkdir # type: ignore - return os.makedirs(file_path, mode) + os.mkdir = orig_mkdir_fn finally: - os.mkdir = orig_mkdir_fn + umask(current_umask) def mkdir(file_path, mode=0o777): @@ -284,7 +315,7 @@ def mkdir(file_path, mode=0o777): attributes = win32security.SECURITY_ATTRIBUTES() security = attributes.SECURITY_DESCRIPTOR user = _get_current_user() - dacl = _generate_dacl(user, mode) + dacl = _generate_dacl(user, mode, _WINDOWS_UMASK.mask) security.SetSecurityDescriptorOwner(user, False) security.SetSecurityDescriptorDacl(1, dacl, 0) @@ -322,6 +353,10 @@ def realpath(file_path): """ Find the real path for the given path. This method resolves symlinks, including recursive symlinks, and is protected against symlinks that creates an infinite loop. + + :param str file_path: The path to resolve + :returns: The real path for the given path + :rtype: str """ original_path = file_path @@ -372,7 +407,7 @@ def is_executable(path): def has_world_permissions(path): # type: (str) -> bool """ - Check if everybody/world has any right (read/write/execute) on a file given its path + Check if everybody/world has any right (read/write/execute) on a file given its path. :param str path: path to test :return: True if everybody/world has any right to the file @@ -394,7 +429,7 @@ def has_world_permissions(path): def compute_private_key_mode(old_key, base_mode): # type: (str, int) -> int """ - Calculate the POSIX mode to apply to a private key given the previous private key + Calculate the POSIX mode to apply to a private key given the previous private key. :param str old_key: path to the previous private key :param int base_mode: the minimum modes to apply to a private key @@ -520,7 +555,9 @@ def _apply_win_mode(file_path, mode): win32security.SetFileSecurity(file_path, win32security.DACL_SECURITY_INFORMATION, security) -def _generate_dacl(user_sid, mode): +def _generate_dacl(user_sid, mode, mask=None): + if mask: + mode = mode & (0o777 - mask) analysis = _analyze_mode(mode) # Get standard accounts from "well-known" sid diff --git a/certbot/certbot/compat/os.py b/certbot/certbot/compat/os.py index 0231dd51a..d262409c1 100644 --- a/certbot/certbot/compat/os.py +++ b/certbot/certbot/compat/os.py @@ -56,6 +56,16 @@ def chmod(*unused_args, **unused_kwargs): 'Use certbot.compat.filesystem.chmod() instead.') +# Since there is no mode on Windows, there is no umask either, and so this method is a noop for +# this platform. In order to have a consistent behavior between Linux and Windows on Certbot files +# and directories, the filesystem umask method must be used instead, since it implements umask for +# Windows. +def umask(*unused_args, **unused_kwargs): + """Method os.chmod() is forbidden""" + raise RuntimeError('Usage of os.umask() is forbidden. ' + 'Use certbot.compat.filesystem.umask() 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. diff --git a/certbot/tests/compat/filesystem_test.py b/certbot/tests/compat/filesystem_test.py index b20ee0e54..262fd02b5 100644 --- a/certbot/tests/compat/filesystem_test.py +++ b/certbot/tests/compat/filesystem_test.py @@ -151,6 +151,57 @@ class WindowsChmodTests(TempDirTestCase): self.assertEqual(security_dacl.GetSecurityDescriptorDacl().GetAceCount(), 2) +class UmaskTest(TempDirTestCase): + def test_umask_on_dir(self): + previous_umask = filesystem.umask(0o022) + + try: + dir1 = os.path.join(self.tempdir, 'probe1') + filesystem.mkdir(dir1) + self.assertTrue(filesystem.check_mode(dir1, 0o755)) + + filesystem.umask(0o077) + + dir2 = os.path.join(self.tempdir, 'dir2') + filesystem.mkdir(dir2) + self.assertTrue(filesystem.check_mode(dir2, 0o700)) + + dir3 = os.path.join(self.tempdir, 'dir3') + filesystem.mkdir(dir3, mode=0o777) + self.assertTrue(filesystem.check_mode(dir3, 0o700)) + finally: + filesystem.umask(previous_umask) + + def test_umask_on_file(self): + previous_umask = filesystem.umask(0o022) + + try: + file1 = os.path.join(self.tempdir, 'probe1') + UmaskTest._create_file(file1) + self.assertTrue(filesystem.check_mode(file1, 0o755)) + + filesystem.umask(0o077) + + file2 = os.path.join(self.tempdir, 'probe2') + UmaskTest._create_file(file2) + self.assertTrue(filesystem.check_mode(file2, 0o700)) + + file3 = os.path.join(self.tempdir, 'probe3') + UmaskTest._create_file(file3) + self.assertTrue(filesystem.check_mode(file3, 0o700)) + finally: + filesystem.umask(previous_umask) + + @staticmethod + def _create_file(path, mode=0o777): + file_desc = None + try: + file_desc = filesystem.open(path, flags=os.O_CREAT, mode=mode) + finally: + if file_desc: + os.close(file_desc) + + class ComputePrivateKeyModeTest(TempDirTestCase): def setUp(self): super(ComputePrivateKeyModeTest, self).setUp() @@ -281,24 +332,21 @@ class WindowsMkdirTests(test_util.TempDirTestCase): self.assertEqual(original_mkdir, std_os.mkdir) -# TODO: This test can be used both by Linux and Windows once on #7967 -@unittest.skipUnless(POSIX_MODE, reason='Needs umask to succeed, and Windows does not have it') -class LinuxMkdirTests(test_util.TempDirTestCase): - """Unit tests for Linux mkdir + makedirs functions in filesystem module""" +class MakedirsTests(test_util.TempDirTestCase): + """Unit tests for makedirs function in filesystem module""" def test_makedirs_correct_permissions(self): path = os.path.join(self.tempdir, 'dir') subpath = os.path.join(path, 'subpath') - previous_umask = os.umask(0o022) + previous_umask = filesystem.umask(0o022) try: filesystem.makedirs(subpath, 0o700) - import os as std_os # pylint: disable=os-module-forbidden - assert stat.S_IMODE(std_os.stat(path).st_mode) == 0o700 - assert stat.S_IMODE(std_os.stat(subpath).st_mode) == 0o700 + assert filesystem.check_mode(path, 0o700) + assert filesystem.check_mode(subpath, 0o700) finally: - os.umask(previous_umask) + filesystem.umask(previous_umask) class CopyOwnershipAndModeTest(test_util.TempDirTestCase): @@ -513,8 +561,8 @@ class IsExecutableTest(test_util.TempDirTestCase): from certbot.compat.filesystem import _generate_dacl - def _execute_mock(user_sid, mode): - dacl = _generate_dacl(user_sid, mode) + def _execute_mock(user_sid, mode, mask=None): + dacl = _generate_dacl(user_sid, mode, mask) for _ in range(1, dacl.GetAceCount()): dacl.DeleteAce(1) # DeleteAce dynamically updates the internal index mapping. return dacl