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/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):

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...

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]

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',

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)
+export CERTBOT_PLUGIN_PATH=$(join : $paths)
-if [ -z "$PYTHONPATH" ]; then
-    unset PYTHONPATH
-fi
 exec certbot "$@"

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.
 

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
 
 

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),

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())

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):
+def execute_command(cmd_name, shell_cmd, env=None):
-    # type: (str, str) -> Tuple[str, str]
+    # 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

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]

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'):

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_any_call("pre-hook", self.dir_hook, env=mock.ANY)
-        mock_execute.assert_called_with("pre-hook", self.config.pre_hook)
+        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_any_call("deploy-hook", self.dir_hook, env=mock.ANY)
-        mock_execute.assert_called_with("deploy-hook", self.config.renew_hook)
+        mock_execute.assert_called_with("deploy-hook", self.config.renew_hook, env=mock.ANY)
 
 
 class ListHooksTest(test_util.TempDirTestCase):

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

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