From de88e7d7778f968d5bcb3d7f950df44977df5e50 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 1 May 2019 02:21:10 +0200 Subject: [PATCH] Implements specific overrides for Fedora 29+ in Apache plugin (#6988) * Start to plug specific logic for Fedora >= 29 * Invert the logic * Implement specifics for Fedora 29 * Fix config * Add documentation * Fix parser, fix tests * Fix import * Fix lint * Use LooseVersion to be fail safe on versions comparison * Remove conditional restart on fedora override * Use parent logic * Update certbot-apache/certbot_apache/tests/fedora_test.py Co-Authored-By: adferrand * Simplify restart test * Update certbot-apache/certbot_apache/override_fedora.py Co-Authored-By: adferrand * Correct test assertion * Fix pylint errors * Revert to a direct call to systemctl --- certbot-apache/certbot_apache/entrypoint.py | 20 +- .../certbot_apache/override_fedora.py | 98 +++++++++ .../certbot_apache/tests/centos_test.py | 8 +- .../certbot_apache/tests/entrypoint_test.py | 8 +- .../certbot_apache/tests/fedora_test.py | 194 ++++++++++++++++++ certbot/util.py | 2 +- 6 files changed, 321 insertions(+), 9 deletions(-) create mode 100644 certbot-apache/certbot_apache/override_fedora.py create mode 100644 certbot-apache/certbot_apache/tests/fedora_test.py diff --git a/certbot-apache/certbot_apache/entrypoint.py b/certbot-apache/certbot_apache/entrypoint.py index 6f1443507..df7297d3e 100644 --- a/certbot-apache/certbot_apache/entrypoint.py +++ b/certbot-apache/certbot_apache/entrypoint.py @@ -1,8 +1,13 @@ """ Entry point for Apache Plugin """ +# Pylint does not like disutils.version when running inside a venv. +# See: https://github.com/PyCQA/pylint/issues/73 +from distutils.version import LooseVersion # pylint: disable=no-name-in-module,import-error + from certbot import util from certbot_apache import configurator from certbot_apache import override_arch +from certbot_apache import override_fedora from certbot_apache import override_darwin from certbot_apache import override_debian from certbot_apache import override_centos @@ -16,7 +21,8 @@ OVERRIDE_CLASSES = { "ubuntu": override_debian.DebianConfigurator, "centos": override_centos.CentOSConfigurator, "centos linux": override_centos.CentOSConfigurator, - "fedora": override_centos.CentOSConfigurator, + "fedora_old": override_centos.CentOSConfigurator, + "fedora": override_fedora.FedoraConfigurator, "ol": override_centos.CentOSConfigurator, "red hat enterprise linux server": override_centos.CentOSConfigurator, "rhel": override_centos.CentOSConfigurator, @@ -27,12 +33,19 @@ OVERRIDE_CLASSES = { "suse": override_suse.OpenSUSEConfigurator, } + def get_configurator(): """ Get correct configurator class based on the OS fingerprint """ - os_info = util.get_os_info() + os_name, os_version = util.get_os_info() + os_name = os_name.lower() override_class = None + + # Special case for older Fedora versions + if os_name == 'fedora' and LooseVersion(os_version) < LooseVersion('29'): + os_name = 'fedora_old' + try: - override_class = OVERRIDE_CLASSES[os_info[0].lower()] + override_class = OVERRIDE_CLASSES[os_name] except KeyError: # OS not found in the list os_like = util.get_systemd_os_like() @@ -45,4 +58,5 @@ def get_configurator(): override_class = configurator.ApacheConfigurator return override_class + ENTRYPOINT = get_configurator() diff --git a/certbot-apache/certbot_apache/override_fedora.py b/certbot-apache/certbot_apache/override_fedora.py new file mode 100644 index 000000000..cb0bf06d0 --- /dev/null +++ b/certbot-apache/certbot_apache/override_fedora.py @@ -0,0 +1,98 @@ +""" Distribution specific override class for Fedora 29+ """ +import pkg_resources +import zope.interface + +from certbot import errors +from certbot import interfaces +from certbot import util + +from certbot_apache import apache_util +from certbot_apache import configurator +from certbot_apache import parser + + +@zope.interface.provider(interfaces.IPluginFactory) +class FedoraConfigurator(configurator.ApacheConfigurator): + """Fedora 29+ specific ApacheConfigurator override class""" + + OS_DEFAULTS = dict( + server_root="/etc/httpd", + vhost_root="/etc/httpd/conf.d", + vhost_files="*.conf", + logs_root="/var/log/httpd", + ctl="httpd", + version_cmd=['httpd', '-v'], + restart_cmd=['apachectl', 'graceful'], + restart_cmd_alt=['apachectl', 'restart'], + conftest_cmd=['apachectl', 'configtest'], + enmod=None, + dismod=None, + le_vhost_ext="-le-ssl.conf", + handle_modules=False, + handle_sites=False, + challenge_location="/etc/httpd/conf.d", + MOD_SSL_CONF_SRC=pkg_resources.resource_filename( + # TODO: eventually newest version of Fedora will need their own config + "certbot_apache", "centos-options-ssl-apache.conf") + ) + + def config_test(self): + """ + Override config_test to mitigate configtest error in vanilla installation + of mod_ssl in Fedora. The error is caused by non-existent self-signed + certificates referenced by the configuration, that would be autogenerated + during the first (re)start of httpd. + """ + try: + super(FedoraConfigurator, self).config_test() + except errors.MisconfigurationError: + self._try_restart_fedora() + + def get_parser(self): + """Initializes the ApacheParser""" + return FedoraParser( + self.aug, self.option("server_root"), self.option("vhost_root"), + self.version, configurator=self) + + def _try_restart_fedora(self): + """ + Tries to restart httpd using systemctl to generate the self signed keypair. + """ + try: + util.run_script(['systemctl', 'restart', 'httpd']) + except errors.SubprocessError as err: + raise errors.MisconfigurationError(str(err)) + + # Finish with actual config check to see if systemctl restart helped + super(FedoraConfigurator, self).config_test() + + def _prepare_options(self): + """ + Override the options dictionary initialization to keep using apachectl + instead of httpd and so take advantages of this new bash script in newer versions + of Fedora to restart httpd. + """ + super(FedoraConfigurator, self)._prepare_options() + self.options["restart_cmd"][0] = 'apachectl' + self.options["restart_cmd_alt"][0] = 'apachectl' + self.options["conftest_cmd"][0] = 'apachectl' + + +class FedoraParser(parser.ApacheParser): + """Fedora 29+ specific ApacheParser override class""" + def __init__(self, *args, **kwargs): + # Fedora 29+ specific configuration file for Apache + self.sysconfig_filep = "/etc/sysconfig/httpd" + super(FedoraParser, self).__init__(*args, **kwargs) + + def update_runtime_variables(self): + """ Override for update_runtime_variables for custom parsing """ + # Opportunistic, works if SELinux not enforced + super(FedoraParser, self).update_runtime_variables() + self._parse_sysconfig_var() + + def _parse_sysconfig_var(self): + """ Parses Apache CLI options from Fedora configuration file """ + defines = apache_util.parse_define_file(self.sysconfig_filep, "OPTIONS") + for k in defines: + self.variables[k] = defines[k] diff --git a/certbot-apache/certbot_apache/tests/centos_test.py b/certbot-apache/certbot_apache/tests/centos_test.py index a0c1636b0..5d16c2b55 100644 --- a/certbot-apache/certbot_apache/tests/centos_test.py +++ b/certbot-apache/certbot_apache/tests/centos_test.py @@ -43,13 +43,14 @@ class FedoraRestartTest(util.ApacheTest): vhost_root=vhost_root) self.config = util.get_apache_configurator( self.config_path, self.vhost_path, self.config_dir, self.work_dir, - os_info="fedora") + os_info="fedora_old") self.vh_truth = get_vh_truth( self.temp_dir, "centos7_apache/apache") def _run_fedora_test(self): + self.assertIsInstance(self.config, override_centos.CentOSConfigurator) with mock.patch("certbot.util.get_os_info") as mock_info: - mock_info.return_value = ["fedora"] + mock_info.return_value = ["fedora", "28"] self.config.config_test() def test_non_fedora_error(self): @@ -103,8 +104,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest): self.temp_dir, "centos7_apache/apache") def test_get_parser(self): - self.assertTrue(isinstance(self.config.parser, - override_centos.CentOSParser)) + self.assertIsInstance(self.config.parser, override_centos.CentOSParser) @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): diff --git a/certbot-apache/certbot_apache/tests/entrypoint_test.py b/certbot-apache/certbot_apache/tests/entrypoint_test.py index 6d85b0db2..9adcd46dc 100644 --- a/certbot-apache/certbot_apache/tests/entrypoint_test.py +++ b/certbot-apache/certbot_apache/tests/entrypoint_test.py @@ -6,6 +6,7 @@ import mock from certbot_apache import configurator from certbot_apache import entrypoint + class EntryPointTest(unittest.TestCase): """Entrypoint tests""" @@ -15,7 +16,12 @@ class EntryPointTest(unittest.TestCase): with mock.patch("certbot.util.get_os_info") as mock_info: for distro in entrypoint.OVERRIDE_CLASSES: - mock_info.return_value = (distro, "whatever") + return_value = (distro, "whatever") + if distro == 'fedora_old': + return_value = ('fedora', '28') + elif distro == 'fedora': + return_value = ('fedora', '29') + mock_info.return_value = return_value self.assertEqual(entrypoint.get_configurator(), entrypoint.OVERRIDE_CLASSES[distro]) diff --git a/certbot-apache/certbot_apache/tests/fedora_test.py b/certbot-apache/certbot_apache/tests/fedora_test.py new file mode 100644 index 000000000..67533fe1d --- /dev/null +++ b/certbot-apache/certbot_apache/tests/fedora_test.py @@ -0,0 +1,194 @@ +"""Test for certbot_apache.configurator for Fedora 29+ overrides""" +import unittest + +import mock + +from certbot import errors +from certbot.compat import os + +from certbot_apache import obj +from certbot_apache import override_fedora +from certbot_apache.tests import util + + +def get_vh_truth(temp_dir, config_name): + """Return the ground truth for the specified directory.""" + prefix = os.path.join( + temp_dir, config_name, "httpd/conf.d") + + aug_pre = "/files" + prefix + # TODO: eventually, these tests should have a dedicated configuration instead + # of reusing the ones from centos_test + vh_truth = [ + obj.VirtualHost( + os.path.join(prefix, "centos.example.com.conf"), + os.path.join(aug_pre, "centos.example.com.conf/VirtualHost"), + {obj.Addr.fromstring("*:80")}, + False, True, "centos.example.com"), + obj.VirtualHost( + os.path.join(prefix, "ssl.conf"), + os.path.join(aug_pre, "ssl.conf/VirtualHost"), + {obj.Addr.fromstring("_default_:443")}, + True, True, None) + ] + return vh_truth + + +class FedoraRestartTest(util.ApacheTest): + """Tests for Fedora specific self-signed certificate override""" + + # TODO: eventually, these tests should have a dedicated configuration instead + # of reusing the ones from centos_test + def setUp(self): # pylint: disable=arguments-differ + test_dir = "centos7_apache/apache" + config_root = "centos7_apache/apache/httpd" + vhost_root = "centos7_apache/apache/httpd/conf.d" + super(FedoraRestartTest, self).setUp(test_dir=test_dir, + config_root=config_root, + vhost_root=vhost_root) + self.config = util.get_apache_configurator( + self.config_path, self.vhost_path, self.config_dir, self.work_dir, + os_info="fedora") + self.vh_truth = get_vh_truth( + self.temp_dir, "centos7_apache/apache") + + def _run_fedora_test(self): + self.assertIsInstance(self.config, override_fedora.FedoraConfigurator) + self.config.config_test() + + def test_fedora_restart_error(self): + c_test = "certbot_apache.configurator.ApacheConfigurator.config_test" + with mock.patch(c_test) as mock_test: + # First call raises error, second doesn't + mock_test.side_effect = [errors.MisconfigurationError, ''] + with mock.patch("certbot.util.run_script") as mock_run: + mock_run.side_effect = errors.SubprocessError + self.assertRaises(errors.MisconfigurationError, + self._run_fedora_test) + + def test_fedora_restart(self): + c_test = "certbot_apache.configurator.ApacheConfigurator.config_test" + with mock.patch(c_test) as mock_test: + with mock.patch("certbot.util.run_script") as mock_run: + # First call raises error, second doesn't + mock_test.side_effect = [errors.MisconfigurationError, ''] + self._run_fedora_test() + self.assertEqual(mock_test.call_count, 2) + self.assertEqual(mock_run.call_args[0][0], + ['systemctl', 'restart', 'httpd']) + + +class MultipleVhostsTestFedora(util.ApacheTest): + """Multiple vhost tests for CentOS / RHEL family of distros""" + + _multiprocess_can_split_ = True + + def setUp(self): # pylint: disable=arguments-differ + test_dir = "centos7_apache/apache" + config_root = "centos7_apache/apache/httpd" + vhost_root = "centos7_apache/apache/httpd/conf.d" + super(MultipleVhostsTestFedora, self).setUp(test_dir=test_dir, + config_root=config_root, + vhost_root=vhost_root) + + self.config = util.get_apache_configurator( + self.config_path, self.vhost_path, self.config_dir, self.work_dir, + os_info="fedora") + self.vh_truth = get_vh_truth( + self.temp_dir, "centos7_apache/apache") + + def test_get_parser(self): + self.assertIsInstance(self.config.parser, override_fedora.FedoraParser) + + @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + def test_opportunistic_httpd_runtime_parsing(self, mock_get): + define_val = ( + 'Define: TEST1\n' + 'Define: TEST2\n' + 'Define: DUMP_RUN_CFG\n' + ) + mod_val = ( + 'Loaded Modules:\n' + ' mock_module (static)\n' + ' another_module (static)\n' + ) + def mock_get_cfg(command): + """Mock httpd process stdout""" + if command == ['httpd', '-t', '-D', 'DUMP_RUN_CFG']: + return define_val + elif command == ['httpd', '-t', '-D', 'DUMP_MODULES']: + return mod_val + return "" + mock_get.side_effect = mock_get_cfg + self.config.parser.modules = set() + self.config.parser.variables = {} + + with mock.patch("certbot.util.get_os_info") as mock_osi: + # Make sure we have the have the CentOS httpd constants + mock_osi.return_value = ("fedora", "29") + self.config.parser.update_runtime_variables() + + self.assertEqual(mock_get.call_count, 3) + self.assertEqual(len(self.config.parser.modules), 4) + self.assertEqual(len(self.config.parser.variables), 2) + self.assertTrue("TEST2" in self.config.parser.variables.keys()) + self.assertTrue("mod_another.c" in self.config.parser.modules) + + @mock.patch("certbot_apache.configurator.util.run_script") + def test_get_version(self, mock_run_script): + mock_run_script.return_value = ('', None) + self.assertRaises(errors.PluginError, self.config.get_version) + self.assertEqual(mock_run_script.call_args[0][0][0], 'httpd') + + def test_get_virtual_hosts(self): + """Make sure all vhosts are being properly found.""" + vhs = self.config.get_virtual_hosts() + self.assertEqual(len(vhs), 2) + found = 0 + + for vhost in vhs: + for centos_truth in self.vh_truth: + if vhost == centos_truth: + found += 1 + break + else: + raise Exception("Missed: %s" % vhost) # pragma: no cover + self.assertEqual(found, 2) + + @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + def test_get_sysconfig_vars(self, mock_cfg): + """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( + os.path.join(self.config.parser.root, "../sysconfig/httpd")) + self.config.parser.variables = {} + + with mock.patch("certbot.util.get_os_info") as mock_osi: + # Make sure we have the have the CentOS httpd constants + mock_osi.return_value = ("fedora", "29") + self.config.parser.update_runtime_variables() + + self.assertTrue("mock_define" in self.config.parser.variables.keys()) + self.assertTrue("mock_define_too" in self.config.parser.variables.keys()) + self.assertTrue("mock_value" in self.config.parser.variables.keys()) + self.assertEqual("TRUE", self.config.parser.variables["mock_value"]) + self.assertTrue("MOCK_NOSEP" in self.config.parser.variables.keys()) + self.assertEqual("NOSEP_VAL", self.config.parser.variables["NOSEP_TWO"]) + + @mock.patch("certbot_apache.configurator.util.run_script") + def test_alt_restart_works(self, mock_run_script): + mock_run_script.side_effect = [None, errors.SubprocessError, None] + self.config.restart() + self.assertEqual(mock_run_script.call_count, 3) + + @mock.patch("certbot_apache.configurator.util.run_script") + def test_alt_restart_errors(self, mock_run_script): + mock_run_script.side_effect = [None, + errors.SubprocessError, + errors.SubprocessError] + self.assertRaises(errors.MisconfigurationError, self.config.restart) + + +if __name__ == "__main__": + unittest.main() # pragma: no cover diff --git a/certbot/util.py b/certbot/util.py index e15d02779..66e5d2524 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -323,7 +323,7 @@ def get_os_info(filepath="/etc/os-release"): # Systemd os-release parsing might be viable os_name, os_version = get_systemd_os_info(filepath=filepath) if os_name: - return (os_name, os_version) + return os_name, os_version # Fallback to platform module return get_python_os_info()