diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index c433c5159..f7c27bf76 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -23,6 +23,7 @@ from certbot import interfaces from certbot import util from certbot.achallenges import KeyAuthorizationAnnotatedChallenge # pylint: disable=unused-import +from certbot.compat import filesystem from certbot.compat import os from certbot.plugins import common from certbot.plugins.util import path_surgery @@ -895,7 +896,7 @@ class ApacheConfigurator(common.Installer): if not new_vhost: continue internal_path = apache_util.get_internal_aug_path(new_vhost.path) - realpath = os.path.realpath(new_vhost.filep) + realpath = filesystem.realpath(new_vhost.filep) if realpath not in file_paths: file_paths[realpath] = new_vhost.filep internal_paths[realpath].add(internal_path) @@ -1221,11 +1222,11 @@ class ApacheConfigurator(common.Installer): """ if self.conf("vhost-root") and os.path.exists(self.conf("vhost-root")): - fp = os.path.join(os.path.realpath(self.option("vhost_root")), + fp = os.path.join(filesystem.realpath(self.option("vhost_root")), os.path.basename(non_ssl_vh_fp)) else: # Use non-ssl filepath - fp = os.path.realpath(non_ssl_vh_fp) + fp = filesystem.realpath(non_ssl_vh_fp) if fp.endswith(".conf"): return fp[:-(len(".conf"))] + self.option("le_vhost_ext") diff --git a/certbot-apache/certbot_apache/override_debian.py b/certbot-apache/certbot_apache/override_debian.py index 5706ce760..58492bd01 100644 --- a/certbot-apache/certbot_apache/override_debian.py +++ b/certbot-apache/certbot_apache/override_debian.py @@ -7,6 +7,7 @@ import zope.interface from certbot import errors from certbot import interfaces from certbot import util +from certbot.compat import filesystem from certbot.compat import os from certbot_apache import apache_util @@ -65,7 +66,7 @@ class DebianConfigurator(configurator.ApacheConfigurator): try: os.symlink(vhost.filep, enabled_path) except OSError as err: - if os.path.islink(enabled_path) and os.path.realpath( + if os.path.islink(enabled_path) and filesystem.realpath( enabled_path) == vhost.filep: # Already in shape vhost.enabled = True diff --git a/certbot-apache/certbot_apache/tests/centos_test.py b/certbot-apache/certbot_apache/tests/centos_test.py index 5d16c2b55..dddbf489e 100644 --- a/certbot-apache/certbot_apache/tests/centos_test.py +++ b/certbot-apache/certbot_apache/tests/centos_test.py @@ -4,6 +4,7 @@ import unittest import mock from certbot import errors +from certbot.compat import filesystem from certbot.compat import os from certbot_apache import obj @@ -160,7 +161,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest): """Make sure we read the sysconfig OPTIONS variable correctly""" # Return nothing for the process calls mock_cfg.return_value = "" - self.config.parser.sysconfig_filep = os.path.realpath( + self.config.parser.sysconfig_filep = filesystem.realpath( os.path.join(self.config.parser.root, "../sysconfig/httpd")) self.config.parser.variables = {} diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index f9b6fb186..1eafae982 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -675,8 +675,7 @@ class MultipleVhostsTest(util.ApacheTest): def test_make_vhost_ssl_nonexistent_vhost_path(self): ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[1]) self.assertEqual(os.path.dirname(ssl_vhost.filep), - os.path.dirname(os.path.realpath( - self.vh_truth[1].filep))) + os.path.dirname(filesystem.realpath(self.vh_truth[1].filep))) def test_make_vhost_ssl(self): ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0]) @@ -1336,7 +1335,7 @@ class MultipleVhostsTest(util.ApacheTest): self.config.parser.modules.add("ssl_module") self.config.parser.modules.add("mod_ssl.c") self.config.parser.modules.add("socache_shmcb_module") - tmp_path = os.path.realpath(tempfile.mkdtemp("vhostroot")) + tmp_path = filesystem.realpath(tempfile.mkdtemp("vhostroot")) filesystem.chmod(tmp_path, 0o755) mock_p = "certbot_apache.configurator.ApacheConfigurator._get_ssl_vhost_path" mock_a = "certbot_apache.parser.ApacheParser.add_include" diff --git a/certbot-apache/certbot_apache/tests/debian_test.py b/certbot-apache/certbot_apache/tests/debian_test.py index f1d5843a5..54ced2d0b 100644 --- a/certbot-apache/certbot_apache/tests/debian_test.py +++ b/certbot-apache/certbot_apache/tests/debian_test.py @@ -79,9 +79,9 @@ class MultipleVhostsTestDebian(util.ApacheTest): def test_enable_site_failure(self): self.config.parser.root = "/tmp/nonexistent" - with mock.patch("os.path.isdir") as mock_dir: + with mock.patch("certbot.compat.os.path.isdir") as mock_dir: mock_dir.return_value = True - with mock.patch("os.path.islink") as mock_link: + with mock.patch("certbot.compat.os.path.islink") as mock_link: mock_link.return_value = False self.assertRaises( errors.NotSupportedError, diff --git a/certbot-apache/certbot_apache/tests/fedora_test.py b/certbot-apache/certbot_apache/tests/fedora_test.py index 67533fe1d..4d3f3a313 100644 --- a/certbot-apache/certbot_apache/tests/fedora_test.py +++ b/certbot-apache/certbot_apache/tests/fedora_test.py @@ -4,6 +4,7 @@ import unittest import mock from certbot import errors +from certbot.compat import filesystem from certbot.compat import os from certbot_apache import obj @@ -160,7 +161,7 @@ class MultipleVhostsTestFedora(util.ApacheTest): """Make sure we read the sysconfig OPTIONS variable correctly""" # Return nothing for the process calls mock_cfg.return_value = "" - self.config.parser.sysconfig_filep = os.path.realpath( + self.config.parser.sysconfig_filep = filesystem.realpath( os.path.join(self.config.parser.root, "../sysconfig/httpd")) self.config.parser.variables = {} diff --git a/certbot-apache/certbot_apache/tests/gentoo_test.py b/certbot-apache/certbot_apache/tests/gentoo_test.py index dd01e9170..d0d3ba0dd 100644 --- a/certbot-apache/certbot_apache/tests/gentoo_test.py +++ b/certbot-apache/certbot_apache/tests/gentoo_test.py @@ -4,6 +4,7 @@ import unittest import mock from certbot import errors +from certbot.compat import filesystem from certbot.compat import os from certbot_apache import obj @@ -81,7 +82,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest): """Make sure we read the Gentoo APACHE2_OPTS variable correctly""" defines = ['DEFAULT_VHOST', 'INFO', 'SSL', 'SSL_DEFAULT_VHOST', 'LANGUAGE'] - self.config.parser.apacheconfig_filep = os.path.realpath( + self.config.parser.apacheconfig_filep = filesystem.realpath( os.path.join(self.config.parser.root, "../conf.d/apache2")) self.config.parser.variables = {} with mock.patch("certbot_apache.override_gentoo.GentooParser.update_modules"): diff --git a/certbot-apache/local-oldest-requirements.txt b/certbot-apache/local-oldest-requirements.txt index a959ad44f..da509406e 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]==0.36.0 +-e .[dev] diff --git a/certbot-apache/setup.py b/certbot-apache/setup.py index 1003ad678..98b7382db 100644 --- a/certbot-apache/setup.py +++ b/certbot-apache/setup.py @@ -10,7 +10,7 @@ version = '0.37.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.29.0', - 'certbot>=0.36.0', + 'certbot>=0.37.0.dev0', 'mock', 'python-augeas', 'setuptools', diff --git a/certbot/compat/_path.py b/certbot/compat/_path.py new file mode 100644 index 000000000..fe2d2d1d2 --- /dev/null +++ b/certbot/compat/_path.py @@ -0,0 +1,31 @@ +"""This compat module wraps os.path to forbid some functions.""" +# pylint: disable=function-redefined +from __future__ import absolute_import + +# First round of wrapping: we import statically all public attributes exposed by the os.path +# module. This allows in particular to have pylint, mypy, IDEs be aware that most of os.path +# members are available in certbot.compat.path. +from os.path import * # type: ignore # pylint: disable=wildcard-import,unused-wildcard-import,redefined-builtin,os-module-forbidden + +# Second round of wrapping: we import dynamically all attributes from the os.path module that have +# not yet been imported by the first round (static star import). +import os.path as std_os_path # pylint: disable=os-module-forbidden +import sys as std_sys + +ourselves = std_sys.modules[__name__] +for attribute in dir(std_os_path): + # Check if the attribute does not already exist in our module. It could be internal attributes + # of the module (__name__, __doc__), or attributes from standard os.path already imported with + # `from os.path import *`. + if not hasattr(ourselves, attribute): + setattr(ourselves, attribute, getattr(std_os_path, attribute)) + +# Clean all remaining importables that are not from the core os.path module. +del ourselves, std_os_path, std_sys + + +# Function os.path.realpath is broken on some versions of Python for Windows. +def realpath(*unused_args, **unused_kwargs): + """Method os.path.realpath() is forbidden""" + raise RuntimeError('Usage of os.path.realpath() is forbidden. ' + 'Use certbot.compat.filesystem.realpath() instead.') diff --git a/certbot/compat/filesystem.py b/certbot/compat/filesystem.py index d0d1c262d..e2757eb2f 100644 --- a/certbot/compat/filesystem.py +++ b/certbot/compat/filesystem.py @@ -207,13 +207,22 @@ def replace(src, dst): os.rename(src, dst) -def _apply_win_mode(file_path, mode): +def realpath(file_path): """ - This function converts the given POSIX mode into a Windows ACL list, and applies it to the - file given its path. If the given path is a symbolic link, it will resolved to apply the - mode on the targeted file. + 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. """ original_path = file_path + + if POSIX_MODE: + path = os.path.realpath(file_path) + if os.path.islink(path): + # If path returned by realpath is still a link, it means that it failed to + # resolve the symlink because of a loop. + # See realpath code: https://github.com/python/cpython/blob/master/Lib/posixpath.py + raise RuntimeError('Error, link {0} is a loop!'.format(original_path)) + return path + inspected_paths = [] # type: List[str] while os.path.islink(file_path): link_path = file_path @@ -223,6 +232,17 @@ def _apply_win_mode(file_path, mode): if file_path in inspected_paths: raise RuntimeError('Error, link {0} is a loop!'.format(original_path)) inspected_paths.append(file_path) + + return os.path.abspath(file_path) + + +def _apply_win_mode(file_path, mode): + """ + This function converts the given POSIX mode into a Windows ACL list, and applies it to the + file given its path. If the given path is a symbolic link, it will resolved to apply the + mode on the targeted file. + """ + file_path = realpath(file_path) # Get owner sid of the file security = win32security.GetFileSecurity(file_path, win32security.OWNER_SECURITY_INFORMATION) user = security.GetSecurityDescriptorOwner() diff --git a/certbot/compat/os.py b/certbot/compat/os.py index 6d3c7bb02..bb0e02eb3 100644 --- a/certbot/compat/os.py +++ b/certbot/compat/os.py @@ -26,7 +26,9 @@ for attribute in dir(std_os): if not hasattr(ourselves, attribute): setattr(ourselves, attribute, getattr(std_os, attribute)) -# Similar to os.path, allow certbot.compat.os.path to behave as a module +# Import our internal path module, then allow certbot.compat.os.path +# to behave as a module (similarly to os.path). +from certbot.compat import _path as path # type: ignore # pylint: disable=wrong-import-position std_sys.modules[__name__ + '.path'] = path # Clean all remaining importables that are not from the core os module. diff --git a/certbot/main.py b/certbot/main.py index 6bd47cee3..50470438f 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -31,6 +31,7 @@ from certbot import reporter from certbot import storage from certbot import updater from certbot import util +from certbot.compat import filesystem from certbot.compat import misc from certbot.compat import os from certbot.display import util as display_util, ops as display_ops @@ -841,12 +842,12 @@ def _populate_from_certname(config): return config def _check_certificate_and_key(config): - if not os.path.isfile(os.path.realpath(config.cert_path)): + if not os.path.isfile(filesystem.realpath(config.cert_path)): raise errors.ConfigurationError("Error while reading certificate from path " - "{0}".format(config.cert_path)) - if not os.path.isfile(os.path.realpath(config.key_path)): + "{0}".format(config.cert_path)) + if not os.path.isfile(filesystem.realpath(config.key_path)): raise errors.ConfigurationError("Error while reading private key from path " - "{0}".format(config.key_path)) + "{0}".format(config.key_path)) def plugins_cmd(config, plugins): """List server software plugins. diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index 2e2e0f64c..5c292bfad 100644 --- a/certbot/plugins/common.py +++ b/certbot/plugins/common.py @@ -486,7 +486,7 @@ def dir_setup(test_dir, pkg): # pragma: no cover link, (ex: OS X) such plugins will be confused. This function prevents such a case. """ - return os.path.realpath(tempfile.mkdtemp(prefix)) + return filesystem.realpath(tempfile.mkdtemp(prefix)) temp_dir = expanded_tempdir("temp") config_dir = expanded_tempdir("config") diff --git a/certbot/plugins/storage_test.py b/certbot/plugins/storage_test.py index f72e69d4b..9d08cc7ef 100644 --- a/certbot/plugins/storage_test.py +++ b/certbot/plugins/storage_test.py @@ -31,7 +31,7 @@ class PluginStorageTest(test_util.ConfigTestCase): self.plugin.storage.storagepath = os.path.join(self.config.config_dir, ".pluginstorage.json") with mock.patch("six.moves.builtins.open", mock_open): - with mock.patch('os.path.isfile', return_value=True): + with mock.patch('certbot.compat.os.path.isfile', return_value=True): with mock.patch("certbot.reverter.util"): self.assertRaises(errors.PluginStorageError, self.plugin.storage._load) # pylint: disable=protected-access diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 30ca6f099..b4cd946ee 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -97,8 +97,8 @@ class UpdateLiveSymlinksTest(BaseCertManagerTest): for kind in ALL_FOUR: os.chdir(os.path.dirname(self.config_files[domain][kind])) self.assertEqual( - os.path.realpath(os.readlink(self.config_files[domain][kind])), - os.path.realpath(archive_paths[domain][kind])) + filesystem.realpath(os.readlink(self.config_files[domain][kind])), + filesystem.realpath(archive_paths[domain][kind])) finally: os.chdir(prev_dir) diff --git a/certbot/tests/compat/filesystem_test.py b/certbot/tests/compat/filesystem_test.py index 27c48c19a..5d112b652 100644 --- a/certbot/tests/compat/filesystem_test.py +++ b/certbot/tests/compat/filesystem_test.py @@ -48,18 +48,6 @@ class WindowsChmodTests(TempDirTestCase): self.assertFalse(filesystem._compare_dacls(ref_dacl_probe, cur_dacl_probe)) # pylint: disable=protected-access self.assertTrue(filesystem._compare_dacls(ref_dacl_link, cur_dacl_link)) # pylint: disable=protected-access - def test_symlink_loop_mitigation(self): - link1_path = os.path.join(self.tempdir, 'link1') - link2_path = os.path.join(self.tempdir, 'link2') - link3_path = os.path.join(self.tempdir, 'link3') - os.symlink(link1_path, link2_path) - os.symlink(link2_path, link3_path) - os.symlink(link3_path, link1_path) - - with self.assertRaises(RuntimeError) as error: - filesystem.chmod(link1_path, 0o755) - self.assertTrue('link1 is a loop!' in str(error.exception)) - def test_world_permission(self): everybody = win32security.ConvertStringSidToSid(EVERYBODY_SID) @@ -320,7 +308,6 @@ class CopyOwnershipTest(test_util.TempDirTestCase): class OsReplaceTest(test_util.TempDirTestCase): """Test to ensure consistent behavior of rename method""" - def test_os_replace_to_existing_file(self): """Ensure that replace will effectively rename src into dst for all platforms.""" src = os.path.join(self.tempdir, 'src') @@ -335,6 +322,46 @@ class OsReplaceTest(test_util.TempDirTestCase): self.assertTrue(os.path.exists(dst)) +class RealpathTest(test_util.TempDirTestCase): + """Tests for realpath method""" + def setUp(self): + super(RealpathTest, self).setUp() + self.probe_path = _create_probe(self.tempdir) + + def test_symlink_resolution(self): + # Absolute resolution + link_path = os.path.join(self.tempdir, 'link_abs') + os.symlink(self.probe_path, link_path) + + self.assertEqual(self.probe_path, filesystem.realpath(self.probe_path)) + self.assertEqual(self.probe_path, filesystem.realpath(link_path)) + + # Relative resolution + curdir = os.getcwd() + link_path = os.path.join(self.tempdir, 'link_rel') + probe_name = os.path.basename(self.probe_path) + try: + os.chdir(os.path.dirname(self.probe_path)) + os.symlink(probe_name, link_path) + + self.assertEqual(self.probe_path, filesystem.realpath(probe_name)) + self.assertEqual(self.probe_path, filesystem.realpath(link_path)) + finally: + os.chdir(curdir) + + def test_symlink_loop_mitigation(self): + link1_path = os.path.join(self.tempdir, 'link1') + link2_path = os.path.join(self.tempdir, 'link2') + link3_path = os.path.join(self.tempdir, 'link3') + os.symlink(link1_path, link2_path) + os.symlink(link2_path, link3_path) + os.symlink(link3_path, link1_path) + + with self.assertRaises(RuntimeError) as error: + filesystem.realpath(link1_path) + self.assertTrue('link1 is a loop!' in str(error.exception)) + + def _get_security_dacl(target): return win32security.GetFileSecurity(target, win32security.DACL_SECURITY_INFORMATION) diff --git a/certbot/tests/compat/os_test.py b/certbot/tests/compat/os_test.py index 9e25548f0..754e97a10 100644 --- a/certbot/tests/compat/os_test.py +++ b/certbot/tests/compat/os_test.py @@ -7,8 +7,12 @@ from certbot.compat import os class OsTest(unittest.TestCase): """Unit tests for os module.""" def test_forbidden_methods(self): + # Checks for os module for method in ['chmod', 'chown', 'open', 'mkdir', 'makedirs', 'rename', 'replace']: self.assertRaises(RuntimeError, getattr(os, method)) + # Checks for os.path module + for method in ['realpath']: + self.assertRaises(RuntimeError, getattr(os.path, method)) if __name__ == "__main__": diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index ebd9a28e1..ff8a800ab 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -542,7 +542,7 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met return True return orig_open(fn) - with mock.patch("os.path.isfile") as mock_if: + with mock.patch("certbot.compat.os.path.isfile") as mock_if: mock_if.side_effect = mock_isfile with mock.patch('certbot.main.client') as client: ret, stdout, stderr = self._call_no_clientmock(args, stdout) diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index 41b5cf71d..b96f8afd2 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -528,7 +528,7 @@ class OsInfoTest(unittest.TestCase): from certbot.util import (get_os_info, get_systemd_os_info, get_os_info_ua) - with mock.patch('os.path.isfile', return_value=True): + with mock.patch('certbot.compat.os.path.isfile', return_value=True): self.assertEqual(get_os_info( test_util.vector_path("os-release"))[0], 'systemdos') self.assertEqual(get_os_info( @@ -536,13 +536,13 @@ class OsInfoTest(unittest.TestCase): self.assertEqual(get_systemd_os_info(os.devnull), ("", "")) self.assertEqual(get_os_info_ua( test_util.vector_path("os-release")), "SystemdOS") - with mock.patch('os.path.isfile', return_value=False): + with mock.patch('certbot.compat.os.path.isfile', return_value=False): self.assertEqual(get_systemd_os_info(), ("", "")) def test_systemd_os_release_like(self): from certbot.util import get_systemd_os_like - with mock.patch('os.path.isfile', return_value=True): + with mock.patch('certbot.compat.os.path.isfile', return_value=True): id_likes = get_systemd_os_like(test_util.vector_path( "os-release")) self.assertEqual(len(id_likes), 3) @@ -552,7 +552,7 @@ class OsInfoTest(unittest.TestCase): def test_non_systemd_os_info(self, popen_mock): from certbot.util import (get_os_info, get_python_os_info, get_os_info_ua) - with mock.patch('os.path.isfile', return_value=False): + with mock.patch('certbot.compat.os.path.isfile', return_value=False): with mock.patch('platform.system_alias', return_value=('NonSystemD', '42', '42')): self.assertEqual(get_os_info()[0], 'nonsystemd')