From 713b91495b4d9ff062afdede19add92a4aa8933d Mon Sep 17 00:00:00 2001 From: ohemorange Date: Thu, 25 Jun 2020 15:36:29 -0700 Subject: [PATCH] Fix paths when calling out to programs outside of snap (#8108) Fixes #8093. This PR modifies and audits all uses of `subprocess` and `Popen` outside of tests, `certbot-ci/`, `certbot-compatibility-test/`, `letsencrypt-auto-source/`, `tools/`, and `windows-installer/`. Calls to outside programs have their `env` modified to remove the `SNAP` components of paths, if they exist. This includes any calls made from hooks, calls to `apachectl` and `nginx`, and to `openssl` from `ocsp.py`. For testing manually, rsync flags will look something like: ``` rsync -avzhe ssh root@focal.domain:/home/certbot/certbot/certbot_*_amd64.snap . rsync -avzhe ssh certbot_*_amd64.snap root@centos7.domain:/root/certbot/ ``` With these modifications, `certbot plugins --prepare` now passes on Centos 7. If I'm wrong and we package the `openssl` binary, the modifications should be removed from `ocsp.py`, and `env` should be passed into `run_script` rather than set internally in its calls from nginx and apache. One caveat with this approach is the disconnect between why it's a problem (packaging) and where it's solved (internal to Certbot). I considered a wrapping approach, but we'd still have to audit specific calls. I think the best way to address this is robust testing; specifically, running the snap on other systems. For hooks, all calls will remove the snap paths if they exist. This is probably fine, because even if the hook intends to call back into certbot, it can do that, it'll just create a new snap. I'm not sure if we need these modifications for the Mac OS X/ Darwin calls, but they can't hurt. * Add method to plugins util to get env without snap paths * Use modified environment in Nginx plugin * Pass through env to certbot.util.run_script * Use modified environment in Apache plugin * move env_no_snap_for_external_calls to certbot.util * Set env internally to run_script, since we use that only to call out * Add env to mac subprocess calls in certbot.util * Add env to openssl call in ocsp.py * Add env for hooks calls in certbot.compat.misc. * Pass env into execute_command to avoid circular dependency * Update hook test to assert called with env * Fix mypy type hint to account for new param * Change signature to include Optional * go back to using CERTBOT_PLUGIN_PATH * no need to modify PYTHONPATH in env * robustly detect when we're in a snap * Improve env util fxn docstring * Update changelog * Add unit tests for env_no_snap_for_external_calls * Import compat.os --- .../certbot_apache/_internal/apache_util.py | 3 +- .../certbot_nginx/_internal/configurator.py | 8 +++-- certbot-nginx/local-oldest-requirements.txt | 2 +- certbot-nginx/setup.py | 2 +- certbot.wrapper | 5 +-- certbot/CHANGELOG.md | 2 ++ certbot/certbot/_internal/hooks.py | 2 +- certbot/certbot/_internal/plugins/disco.py | 8 +++++ certbot/certbot/_internal/plugins/manual.py | 4 ++- certbot/certbot/compat/misc.py | 12 ++++--- certbot/certbot/ocsp.py | 3 +- certbot/certbot/util.py | 30 +++++++++++++++- certbot/tests/hook_test.py | 24 ++++++------- certbot/tests/util_test.py | 34 +++++++++++++++++++ snap/snapcraft.yaml | 2 ++ 15 files changed, 110 insertions(+), 31 deletions(-) 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