diff --git a/certbot-apache/certbot_apache/_internal/apache_util.py b/certbot-apache/certbot_apache/_internal/apache_util.py index 862685027..93612f424 100644 --- a/certbot-apache/certbot_apache/_internal/apache_util.py +++ b/certbot-apache/certbot_apache/_internal/apache_util.py @@ -225,7 +225,8 @@ def _get_runtime_cfg(command): command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, - universal_newlines=True) + universal_newlines=True, + env=util.env_no_snap_for_external_calls()) stdout, stderr = proc.communicate() except (OSError, ValueError): diff --git a/certbot-nginx/certbot_nginx/_internal/configurator.py b/certbot-nginx/certbot_nginx/_internal/configurator.py index a903c12bf..a70572c33 100644 --- a/certbot-nginx/certbot_nginx/_internal/configurator.py +++ b/certbot-nginx/certbot_nginx/_internal/configurator.py @@ -939,7 +939,8 @@ class NginxConfigurator(common.Installer): [self.conf('ctl'), "-c", self.nginx_conf, "-V"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, - universal_newlines=True) + universal_newlines=True, + env=util.env_no_snap_for_external_calls()) text = proc.communicate()[1] # nginx prints output to stderr except (OSError, ValueError) as error: logger.debug(str(error), exc_info=True) @@ -1169,7 +1170,8 @@ def nginx_restart(nginx_ctl, nginx_conf): """ try: - proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf, "-s", "reload"]) + proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf, "-s", "reload"], + env=util.env_no_snap_for_external_calls()) proc.communicate() if proc.returncode != 0: @@ -1179,7 +1181,7 @@ def nginx_restart(nginx_ctl, nginx_conf): with tempfile.TemporaryFile() as out: with tempfile.TemporaryFile() as err: nginx_proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf], - stdout=out, stderr=err) + stdout=out, stderr=err, env=util.env_no_snap_for_external_calls()) nginx_proc.communicate() if nginx_proc.returncode != 0: # Enter recovery routine... diff --git a/certbot-nginx/local-oldest-requirements.txt b/certbot-nginx/local-oldest-requirements.txt index e6d9f26fd..aa0aaa0c8 100644 --- a/certbot-nginx/local-oldest-requirements.txt +++ b/certbot-nginx/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==1.4.0 -certbot[dev]==1.4.0 +-e certbot[dev] diff --git a/certbot-nginx/setup.py b/certbot-nginx/setup.py index 073a1a4d2..48e399edb 100644 --- a/certbot-nginx/setup.py +++ b/certbot-nginx/setup.py @@ -12,7 +12,7 @@ version = '1.6.0.dev0' # acme/certbot version. install_requires = [ 'acme>=1.4.0', - 'certbot>=1.4.0', + 'certbot>=1.6.0.dev0', 'PyOpenSSL', 'pyparsing>=1.5.5', # Python3 support 'setuptools', diff --git a/certbot.wrapper b/certbot.wrapper index e5901b8ea..c779e9939 100755 --- a/certbot.wrapper +++ b/certbot.wrapper @@ -12,8 +12,5 @@ join() { } paths=$(for plugin_snap in $(snap connections certbot|sed -n '2,$p'|awk '$1=="content[certbot-1]"{print $3}'|cut -d: -f1); do echo /snap/$plugin_snap/current/lib/python3.8/site-packages; done) -export PYTHONPATH=$(join : $PYTHONPATH $paths) -if [ -z "$PYTHONPATH" ]; then - unset PYTHONPATH -fi +export CERTBOT_PLUGIN_PATH=$(join : $paths) exec certbot "$@" diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 41e6d621a..57e273fe1 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -29,6 +29,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). to fix some packaging issues with libraries respecting PEP404 for version string, with doesn't match `StrictVersion` requirements. * Certbot output doesn't refer to SSL Labs due to confusing scoring behavior. +* Fix paths when calling to programs outside of the Certbot Snap, fixing the apache and nginx + plugins on, e.g., CentOS 7. More details about these changes can be found on our GitHub repo. diff --git a/certbot/certbot/_internal/hooks.py b/certbot/certbot/_internal/hooks.py index f26c5c76b..5526b21c4 100644 --- a/certbot/certbot/_internal/hooks.py +++ b/certbot/certbot/_internal/hooks.py @@ -228,7 +228,7 @@ def _run_hook(cmd_name, shell_cmd): :type shell_cmd: `list` of `str` or `str` :returns: stderr if there was any""" - err, _ = misc.execute_command(cmd_name, shell_cmd) + err, _ = misc.execute_command(cmd_name, shell_cmd, env=util.env_no_snap_for_external_calls()) return err diff --git a/certbot/certbot/_internal/plugins/disco.py b/certbot/certbot/_internal/plugins/disco.py index f1d89f06a..4cce895d8 100644 --- a/certbot/certbot/_internal/plugins/disco.py +++ b/certbot/certbot/_internal/plugins/disco.py @@ -2,6 +2,7 @@ import collections import itertools import logging +import sys import pkg_resources import six @@ -12,6 +13,7 @@ from acme.magic_typing import Dict from certbot import errors from certbot import interfaces from certbot._internal import constants +from certbot.compat import os try: # Python 3.3+ @@ -198,6 +200,12 @@ class PluginsRegistry(Mapping): def find_all(cls): """Find plugins using setuptools entry points.""" plugins = {} # type: Dict[str, PluginEntryPoint] + plugin_paths_string = os.getenv('CERTBOT_PLUGIN_PATH') + plugin_paths = plugin_paths_string.split(':') if plugin_paths_string else [] + # XXX should ensure this only happens once + sys.path.extend(plugin_paths) + for plugin_path in plugin_paths: + pkg_resources.working_set.add_entry(plugin_path) entry_points = itertools.chain( pkg_resources.iter_entry_points( constants.SETUPTOOLS_PLUGINS_ENTRY_POINT), diff --git a/certbot/certbot/_internal/plugins/manual.py b/certbot/certbot/_internal/plugins/manual.py index 430059445..48bb62632 100644 --- a/certbot/certbot/_internal/plugins/manual.py +++ b/certbot/certbot/_internal/plugins/manual.py @@ -8,6 +8,7 @@ from certbot import achallenges # pylint: disable=unused-import from certbot import errors from certbot import interfaces from certbot import reverter +from certbot import util from certbot._internal import hooks from certbot.compat import misc from certbot.compat import os @@ -187,4 +188,5 @@ permitted by DNS standards.) self.reverter.recovery_routine() def _execute_hook(self, hook_name): - return misc.execute_command(self.option_name(hook_name), self.conf(hook_name)) + return misc.execute_command(self.option_name(hook_name), self.conf(hook_name), + env=util.env_no_snap_for_external_calls()) diff --git a/certbot/certbot/compat/misc.py b/certbot/certbot/compat/misc.py index 49a0814f4..f4ea4a5cc 100644 --- a/certbot/certbot/compat/misc.py +++ b/certbot/certbot/compat/misc.py @@ -12,7 +12,7 @@ import sys from certbot import errors from certbot.compat import os -from acme.magic_typing import Tuple +from acme.magic_typing import Tuple, Optional try: from win32com.shell import shell as shellwin32 @@ -116,8 +116,8 @@ def underscores_for_unsupported_characters_in_path(path): return drive + tail.replace(':', '_') -def execute_command(cmd_name, shell_cmd): - # type: (str, str) -> Tuple[str, str] +def execute_command(cmd_name, shell_cmd, env=None): + # type: (str, str, Optional[dict]) -> Tuple[str, str] """ Run a command: - on Linux command will be run by the standard shell selected with Popen(shell=True) @@ -125,6 +125,7 @@ def execute_command(cmd_name, shell_cmd): :param str cmd_name: the user facing name of the hook being run :param str shell_cmd: shell command to execute + :param dict env: environ to pass into Popen :returns: `tuple` (`str` stderr, `str` stdout) """ @@ -132,11 +133,12 @@ def execute_command(cmd_name, shell_cmd): if POSIX_MODE: cmd = subprocess.Popen(shell_cmd, shell=True, stdout=subprocess.PIPE, - stderr=subprocess.PIPE, universal_newlines=True) + stderr=subprocess.PIPE, universal_newlines=True, + env=env) else: line = ['powershell.exe', '-Command', shell_cmd] cmd = subprocess.Popen(line, stdout=subprocess.PIPE, stderr=subprocess.PIPE, - universal_newlines=True) + universal_newlines=True, env=env) # universal_newlines causes Popen.communicate() # to return str objects instead of bytes in Python 3 diff --git a/certbot/certbot/ocsp.py b/certbot/certbot/ocsp.py index 51ada012a..75ce9e2ff 100644 --- a/certbot/certbot/ocsp.py +++ b/certbot/certbot/ocsp.py @@ -51,7 +51,8 @@ class RevocationChecker(object): # New versions of openssl want -header var=val, old ones want -header var val test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], - stdout=PIPE, stderr=PIPE, universal_newlines=True) + stdout=PIPE, stderr=PIPE, universal_newlines=True, + env=util.env_no_snap_for_external_calls()) _out, err = test_host_format.communicate() if "Missing =" in err: self.host_args = lambda host: ["Host=" + host] diff --git a/certbot/certbot/util.py b/certbot/certbot/util.py index e69b11543..33e2a3b32 100644 --- a/certbot/certbot/util.py +++ b/certbot/certbot/util.py @@ -61,6 +61,31 @@ _INITIAL_PID = os.getpid() _LOCKS = OrderedDict() # type: OrderedDict[str, lock.LockFile] +def env_no_snap_for_external_calls(): + """ + When Certbot is run inside a Snap, certain environment variables + are modified. But Certbot sometimes calls out to external programs, + since it uses classic confinement. When we do that, we must modify + the env to remove our modifications so it will use the system's + libraries, since they may be incompatible with the versions of + libraries included in the Snap. For example, apachectl, Nginx, and + anything run from inside a hook should call this function and pass + the results into the ``env`` argument of ``subprocess.Popen``. + + :returns: A modified copy of os.environ ready to pass to Popen + :rtype: dict + + """ + env = os.environ.copy() + # Avoid accidentally modifying env + if 'SNAP' not in env or 'CERTBOT_SNAPPED' not in env: + return env + for path_name in ('PATH', 'LD_LIBRARY_PATH'): + if path_name in env: + env[path_name] = ':'.join(x for x in env[path_name].split(':') if env['SNAP'] not in x) + return env + + def run_script(params, log=logger.error): """Run the script with the given params. @@ -72,7 +97,8 @@ def run_script(params, log=logger.error): proc = subprocess.Popen(params, stdout=subprocess.PIPE, stderr=subprocess.PIPE, - universal_newlines=True) + universal_newlines=True, + env=env_no_snap_for_external_calls()) except (OSError, ValueError): msg = "Unable to run the command: %s" % " ".join(params) @@ -377,12 +403,14 @@ def get_python_os_info(pretty=False): ["/usr/bin/sw_vers", "-productVersion"], stdout=subprocess.PIPE, universal_newlines=True, + env=env_no_snap_for_external_calls(), ) except OSError: proc = subprocess.Popen( ["sw_vers", "-productVersion"], stdout=subprocess.PIPE, universal_newlines=True, + env=env_no_snap_for_external_calls(), ) os_ver = proc.communicate()[0].rstrip('\n') elif os_type.startswith('freebsd'): diff --git a/certbot/tests/hook_test.py b/certbot/tests/hook_test.py index b8fd02461..06a641216 100644 --- a/certbot/tests/hook_test.py +++ b/certbot/tests/hook_test.py @@ -122,7 +122,7 @@ class PreHookTest(HookTest): def _test_nonrenew_common(self): mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with("pre-hook", self.config.pre_hook) + mock_execute.assert_called_once_with("pre-hook", self.config.pre_hook, env=mock.ANY) self._test_no_executions_common() def test_no_hooks(self): @@ -138,21 +138,21 @@ class PreHookTest(HookTest): def test_renew_disabled_dir_hooks(self): self.config.directory_hooks = False mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with("pre-hook", self.config.pre_hook) + mock_execute.assert_called_once_with("pre-hook", self.config.pre_hook, env=mock.ANY) self._test_no_executions_common() def test_renew_no_overlap(self): self.config.verb = "renew" mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_any_call("pre-hook", self.dir_hook) - mock_execute.assert_called_with("pre-hook", self.config.pre_hook) + mock_execute.assert_any_call("pre-hook", self.dir_hook, env=mock.ANY) + mock_execute.assert_called_with("pre-hook", self.config.pre_hook, env=mock.ANY) self._test_no_executions_common() def test_renew_with_overlap(self): self.config.pre_hook = self.dir_hook self.config.verb = "renew" mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with("pre-hook", self.dir_hook) + mock_execute.assert_called_once_with("pre-hook", self.dir_hook, env=mock.ANY) self._test_no_executions_common() def _test_no_executions_common(self): @@ -194,7 +194,7 @@ class PostHookTest(HookTest): for verb in ("certonly", "run",): self.config.verb = verb mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with("post-hook", self.config.post_hook) + mock_execute.assert_called_once_with("post-hook", self.config.post_hook, env=mock.ANY) self.assertFalse(self._get_eventually()) def test_cert_only_and_run_without_hook(self): @@ -283,7 +283,7 @@ class RunSavedPostHooksTest(HookTest): def test_single(self): self.eventually = ["foo"] mock_execute = self._call_with_mock_execute_and_eventually() - mock_execute.assert_called_once_with("post-hook", self.eventually[0]) + mock_execute.assert_called_once_with("post-hook", self.eventually[0], env=mock.ANY) class RenewalHookTest(HookTest): @@ -361,7 +361,7 @@ class DeployHookTest(RenewalHookTest): self.config.deploy_hook = "foo" mock_execute = self._call_with_mock_execute( self.config, domains, lineage) - mock_execute.assert_called_once_with("deploy-hook", self.config.deploy_hook) + mock_execute.assert_called_once_with("deploy-hook", self.config.deploy_hook, env=mock.ANY) class RenewHookTest(RenewalHookTest): @@ -385,7 +385,7 @@ class RenewHookTest(RenewalHookTest): self.config.directory_hooks = False mock_execute = self._call_with_mock_execute( self.config, ["example.org"], "/foo/bar") - mock_execute.assert_called_once_with("deploy-hook", self.config.renew_hook) + mock_execute.assert_called_once_with("deploy-hook", self.config.renew_hook, env=mock.ANY) @mock.patch("certbot._internal.hooks.logger") def test_dry_run(self, mock_logger): @@ -409,13 +409,13 @@ class RenewHookTest(RenewalHookTest): self.config.renew_hook = self.dir_hook mock_execute = self._call_with_mock_execute( self.config, ["example.net", "example.org"], "/foo/bar") - mock_execute.assert_called_once_with("deploy-hook", self.dir_hook) + mock_execute.assert_called_once_with("deploy-hook", self.dir_hook, env=mock.ANY) def test_no_overlap(self): mock_execute = self._call_with_mock_execute( self.config, ["example.org"], "/foo/bar") - mock_execute.assert_any_call("deploy-hook", self.dir_hook) - mock_execute.assert_called_with("deploy-hook", self.config.renew_hook) + mock_execute.assert_any_call("deploy-hook", self.dir_hook, env=mock.ANY) + mock_execute.assert_called_with("deploy-hook", self.config.renew_hook, env=mock.ANY) class ListHooksTest(test_util.TempDirTestCase): diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index 6dd0f964c..d28d7df6f 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -17,6 +17,40 @@ from certbot.compat import os import certbot.tests.util as test_util +class EnvNoSnapForExternalCallsTest(unittest.TestCase): + """Tests for certbot.util.env_no_snap_for_external_calls.""" + @classmethod + def _call(cls): + from certbot.util import env_no_snap_for_external_calls + return env_no_snap_for_external_calls() + + def test_removed(self): + original_path = os.environ['PATH'] + env_copy_dict = os.environ.copy() + env_copy_dict['PATH'] = 'RANDOM_NONSENSE_GARBAGE/blah/blah:' + original_path + env_copy_dict['SNAP'] = 'RANDOM_NONSENSE_GARBAGE' + env_copy_dict['CERTBOT_SNAPPED'] = 'True' + with mock.patch('certbot.compat.os.environ.copy', return_value=env_copy_dict): + self.assertEqual(self._call()['PATH'], original_path) + + def test_noop(self): + env_copy_dict_unmodified = os.environ.copy() + env_copy_dict_unmodified['PATH'] = 'RANDOM_NONSENSE_GARBAGE/blah/blah:' \ + + env_copy_dict_unmodified['PATH'] + env_copy_dict = env_copy_dict_unmodified.copy() + with mock.patch('certbot.compat.os.environ.copy', return_value=env_copy_dict): + # contains neither necessary key + env_copy_dict.pop('SNAP', None) + env_copy_dict.pop('CERTBOT_SNAPPED', None) + self.assertEqual(self._call()['PATH'], env_copy_dict_unmodified['PATH']) + # contains only one necessary key + env_copy_dict['SNAP'] = 'RANDOM_NONSENSE_GARBAGE' + self.assertEqual(self._call()['PATH'], env_copy_dict_unmodified['PATH']) + del env_copy_dict['SNAP'] + env_copy_dict['CERTBOT_SNAPPED'] = 'True' + self.assertEqual(self._call()['PATH'], env_copy_dict_unmodified['PATH']) + + class RunScriptTest(unittest.TestCase): """Tests for certbot.util.run_script.""" @classmethod diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml index e958939b3..14c1fc9e7 100644 --- a/snap/snapcraft.yaml +++ b/snap/snapcraft.yaml @@ -25,6 +25,7 @@ apps: PATH: "$SNAP/bin:$SNAP/usr/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games" AUGEAS_LENS_LIB: "$SNAP/usr/share/augeas/lenses/dist" LD_LIBRARY_PATH: "$SNAP/usr/lib/x86_64-linux-gnu/:$LD_LIBRARY_PATH" + CERTBOT_SNAPPED: "True" renew: command: certbot.wrapper -q renew daemon: oneshot @@ -32,6 +33,7 @@ apps: PATH: "$SNAP/bin:$SNAP/usr/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games" AUGEAS_LENS_LIB: $SNAP/usr/share/augeas/lenses/dist LD_LIBRARY_PATH: "$SNAP/usr/lib/x86_64-linux-gnu/:$LD_LIBRARY_PATH" + CERTBOT_SNAPPED: "True" # Run approximately twice a day with randomization timer: 00:00~24:00/2