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 <adferrand@users.noreply.github.com>

* Simplify restart test

* Update certbot-apache/certbot_apache/override_fedora.py

Co-Authored-By: adferrand <adferrand@users.noreply.github.com>

* Correct test assertion

* Fix pylint errors

* Revert to a direct call to systemctl

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

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]

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

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

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

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