certbot-apache: use httpd by default for CentOS/RHEL (#9402)

* certbot-apache: use httpd for newer RHEL derived distros

A change in RHEL 9 is causing apachectl to error out when used
with additional arguments, resulting in certbot errors. The CentOS
configurator now uses httpd instead for RHEL 9 (and later) derived
distros.

* Single CentOS class which uses the apache_bin option

* soothe mypy

* Always call super()._override_cmds()

7 files changed, 117 insertions, 37 deletions

diff --git a/certbot-apache/certbot_apache/_internal/apache_util.py b/certbot-apache/certbot_apache/_internal/apache_util.py
index a538d8f0d..54b9f9824 100644
--- a/certbot-apache/certbot_apache/_internal/apache_util.py
+++ b/certbot-apache/certbot_apache/_internal/apache_util.py
@@ -136,20 +136,18 @@ def included_in_paths(filepath: str, paths: Iterable[str]) -> bool:
     return any(fnmatch.fnmatch(filepath, path) for path in paths)
 
 
-def parse_defines(apachectl: str) -> Dict[str, str]:
+def parse_defines(define_cmd: List[str]) -> Dict[str, str]:
     """
     Gets Defines from httpd process and returns a dictionary of
     the defined variables.
 
-    :param str apachectl: Path to apachectl executable
+    :param list define_cmd: httpd command to dump defines
 
     :returns: dictionary of defined variables
     :rtype: dict
     """
 
     variables: Dict[str, str] = {}
-    define_cmd = [apachectl, "-t", "-D",
-                  "DUMP_RUN_CFG"]
     matches = parse_from_subprocess(define_cmd, r"Define: ([^ \n]*)")
     try:
         matches.remove("DUMP_RUN_CFG")
@@ -165,33 +163,31 @@ def parse_defines(apachectl: str) -> Dict[str, str]:
     return variables
 
 
-def parse_includes(apachectl: str) -> List[str]:
+def parse_includes(inc_cmd: List[str]) -> List[str]:
     """
     Gets Include directives from httpd process and returns a list of
     their values.
 
-    :param str apachectl: Path to apachectl executable
+    :param list inc_cmd: httpd command to dump includes
 
     :returns: list of found Include directive values
     :rtype: list of str
     """
 
-    inc_cmd: List[str] = [apachectl, "-t", "-D", "DUMP_INCLUDES"]
     return parse_from_subprocess(inc_cmd, r"\(.*\) (.*)")
 
 
-def parse_modules(apachectl: str) -> List[str]:
+def parse_modules(mod_cmd: List[str]) -> List[str]:
     """
     Get loaded modules from httpd process, and return the list
     of loaded module names.
 
-    :param str apachectl: Path to apachectl executable
+    :param list mod_cmd: httpd command to dump loaded modules
 
     :returns: list of found LoadModule module names
     :rtype: list of str
     """
 
-    mod_cmd = [apachectl, "-t", "-D", "DUMP_MODULES"]
     return parse_from_subprocess(mod_cmd, r"(.*)_module")
 
 
diff --git a/certbot-apache/certbot_apache/_internal/configurator.py b/certbot-apache/certbot_apache/_internal/configurator.py
index 82b3dcaec..db9eea444 100644
--- a/certbot-apache/certbot_apache/_internal/configurator.py
+++ b/certbot-apache/certbot_apache/_internal/configurator.py
@@ -85,6 +85,10 @@ class OsOptions:
         self.restart_cmd = ['apache2ctl', 'graceful'] if not restart_cmd else restart_cmd
         self.restart_cmd_alt = restart_cmd_alt
         self.conftest_cmd = ['apache2ctl', 'configtest'] if not conftest_cmd else conftest_cmd
+        syntax_tests_cmd_base = [ctl, '-t', '-D']
+        self.get_defines_cmd = syntax_tests_cmd_base + ['DUMP_RUN_CFG']
+        self.get_includes_cmd = syntax_tests_cmd_base + ['DUMP_INCLUDES']
+        self.get_modules_cmd = syntax_tests_cmd_base + ['DUMP_MODULES']
         self.enmod = enmod
         self.dismod = dismod
         self.le_vhost_ext = le_vhost_ext
@@ -166,6 +170,17 @@ class ApacheConfigurator(common.Configurator):
             return apache_util.find_ssl_apache_conf("old")
         return apache_util.find_ssl_apache_conf("current")
 
+    def _override_cmds(self) -> None:
+        """
+        Set our various command binaries to whatever the user has overridden for apachectl
+        """
+        self.options.version_cmd[0] = self.options.ctl
+        self.options.restart_cmd[0] = self.options.ctl
+        self.options.conftest_cmd[0] = self.options.ctl
+        self.options.get_modules_cmd[0] = self.options.ctl
+        self.options.get_includes_cmd[0] = self.options.ctl
+        self.options.get_defines_cmd[0] = self.options.ctl
+
     def _prepare_options(self) -> None:
         """
         Set the values possibly changed by command line parameters to
@@ -181,10 +196,7 @@ class ApacheConfigurator(common.Configurator):
             else:
                 setattr(self.options, o, getattr(self.OS_DEFAULTS, o))
 
-        # Special cases
-        self.options.version_cmd[0] = self.options.ctl
-        self.options.restart_cmd[0] = self.options.ctl
-        self.options.conftest_cmd[0] = self.options.ctl
+        self._override_cmds()
 
     @classmethod
     def add_parser_arguments(cls, add: Callable[..., None]) -> None:
@@ -479,9 +491,9 @@ class ApacheConfigurator(common.Configurator):
 
         if HAS_APACHECONFIG:
             apache_vars = {
-                "defines": apache_util.parse_defines(self.options.ctl),
-                "includes": apache_util.parse_includes(self.options.ctl),
-                "modules": apache_util.parse_modules(self.options.ctl),
+                "defines": apache_util.parse_defines(self.options.get_defines_cmd),
+                "includes": apache_util.parse_includes(self.options.get_includes_cmd),
+                "modules": apache_util.parse_modules(self.options.get_modules_cmd),
             }
             metadata["apache_vars"] = apache_vars
 
diff --git a/certbot-apache/certbot_apache/_internal/override_centos.py b/certbot-apache/certbot_apache/_internal/override_centos.py
index a436e8457..583d30b98 100644
--- a/certbot-apache/certbot_apache/_internal/override_centos.py
+++ b/certbot-apache/certbot_apache/_internal/override_centos.py
@@ -25,6 +25,7 @@ class CentOSConfigurator(configurator.ApacheConfigurator):
         vhost_files="*.conf",
         logs_root="/var/log/httpd",
         ctl="apachectl",
+        apache_bin="httpd",
         version_cmd=['apachectl', '-v'],
         restart_cmd=['apachectl', 'graceful'],
         restart_cmd_alt=['apachectl', 'restart'],
@@ -51,6 +52,37 @@ class CentOSConfigurator(configurator.ApacheConfigurator):
             else:
                 raise
 
+    def _rhel9_or_newer(self) -> bool:
+        os_name, os_version = util.get_os_info()
+        rhel_derived = os_name in [
+            "centos", "centos linux",
+            "cloudlinux",
+            "ol", "oracle",
+            "rhel", "redhatenterpriseserver", "red hat enterprise linux server",
+            "scientific", "scientific linux",
+        ]
+        at_least_v9 = util.parse_loose_version(os_version) >= util.parse_loose_version('9')
+        return rhel_derived and at_least_v9
+
+    def _override_cmds(self) -> None:
+        super()._override_cmds()
+
+        # As of RHEL 9, apachectl can't be passed flags like "-v" or "-t -D", so
+        # instead use options.bin (i.e. httpd) for version_cmd and the various
+        # get_X commands
+        if self._rhel9_or_newer():
+            if not self.options.bin:
+                raise ValueError("OS option apache_bin must be set for CentOS") # pragma: no cover
+
+            self.options.version_cmd[0] = self.options.bin
+            self.options.get_modules_cmd[0] = self.options.bin
+            self.options.get_includes_cmd[0] = self.options.bin
+            self.options.get_defines_cmd[0] = self.options.bin
+
+        if not self.options.restart_cmd_alt:  # pragma: no cover
+            raise ValueError("OS option restart_cmd_alt must be set for CentOS.")
+        self.options.restart_cmd_alt[0] = self.options.ctl
+
     def _try_restart_fedora(self) -> None:
         """
         Tries to restart httpd using systemctl to generate the self signed key pair.
@@ -64,16 +96,6 @@ class CentOSConfigurator(configurator.ApacheConfigurator):
         # Finish with actual config check to see if systemctl restart helped
         super().config_test()
 
-    def _prepare_options(self) -> None:
-        """
-        Override the options dictionary initialization in order to support
-        alternative restart cmd used in CentOS.
-        """
-        super()._prepare_options()
-        if not self.options.restart_cmd_alt:  # pragma: no cover
-            raise ValueError("OS option restart_cmd_alt must be set for CentOS.")
-        self.options.restart_cmd_alt[0] = self.options.ctl
-
     def get_parser(self) -> "CentOSParser":
         """Initializes the ApacheParser"""
         return CentOSParser(
diff --git a/certbot-apache/certbot_apache/_internal/parser.py b/certbot-apache/certbot_apache/_internal/parser.py
index 46e61843f..019172d37 100644
--- a/certbot-apache/certbot_apache/_internal/parser.py
+++ b/certbot-apache/certbot_apache/_internal/parser.py
@@ -302,7 +302,7 @@ class ApacheParser:
 
     def update_defines(self) -> None:
         """Updates the dictionary of known variables in the configuration"""
-        self.variables = apache_util.parse_defines(self.configurator.options.ctl)
+        self.variables = apache_util.parse_defines(self.configurator.options.get_defines_cmd)
 
     def update_includes(self) -> None:
         """Get includes from httpd process, and add them to DOM if needed"""
@@ -312,7 +312,7 @@ class ApacheParser:
         # configuration files
         _ = self.find_dir("Include")
 
-        matches = apache_util.parse_includes(self.configurator.options.ctl)
+        matches = apache_util.parse_includes(self.configurator.options.get_includes_cmd)
         if matches:
             for i in matches:
                 if not self.parsed_in_current(i):
@@ -321,7 +321,7 @@ class ApacheParser:
     def update_modules(self) -> None:
         """Get loaded modules from httpd process, and add them to DOM"""
 
-        matches = apache_util.parse_modules(self.configurator.options.ctl)
+        matches = apache_util.parse_modules(self.configurator.options.get_modules_cmd)
         for mod in matches:
             self.add_mod(mod.strip())
 
diff --git a/certbot-apache/tests/centos_test.py b/certbot-apache/tests/centos_test.py
index c9a820466..a9a7d8dcc 100644
--- a/certbot-apache/tests/centos_test.py
+++ b/certbot-apache/tests/centos_test.py
@@ -9,8 +9,8 @@ except ImportError: # pragma: no cover
 from certbot import errors
 from certbot.compat import filesystem
 from certbot.compat import os
-from certbot_apache._internal import obj
 from certbot_apache._internal import override_centos
+from certbot_apache._internal import obj
 import util
 
 
@@ -88,19 +88,66 @@ class FedoraRestartTest(util.ApacheTest):
                                 ['systemctl', 'restart', 'httpd'])
 
 
+class UseCorrectApacheExecutableTest(util.ApacheTest):
+    """Make sure the various CentOS/RHEL versions use the right httpd executable"""
+
+    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().setUp(test_dir=test_dir,
+                      config_root=config_root,
+                      vhost_root=vhost_root)
+
+    @mock.patch("certbot.util.get_os_info")
+    def test_old_centos_rhel_and_fedora(self, mock_get_os_info):
+        for os_info in [("centos", "7"), ("rhel", "7"), ("fedora", "28"), ("scientific", "6")]:
+            mock_get_os_info.return_value = os_info
+            config = util.get_apache_configurator(
+                self.config_path, self.vhost_path, self.config_dir, self.work_dir,
+                os_info="centos")
+            self.assertEqual(config.options.ctl, "apachectl")
+            self.assertEqual(config.options.bin, "httpd")
+            self.assertEqual(config.options.version_cmd, ["apachectl", "-v"])
+            self.assertEqual(config.options.restart_cmd, ["apachectl", "graceful"])
+            self.assertEqual(config.options.restart_cmd_alt, ["apachectl", "restart"])
+            self.assertEqual(config.options.conftest_cmd, ["apachectl", "configtest"])
+            self.assertEqual(config.options.get_defines_cmd, ["apachectl", "-t", "-D", "DUMP_RUN_CFG"])
+            self.assertEqual(config.options.get_includes_cmd, ["apachectl", "-t", "-D", "DUMP_INCLUDES"])
+            self.assertEqual(config.options.get_modules_cmd, ["apachectl", "-t", "-D", "DUMP_MODULES"])
+
+    @mock.patch("certbot.util.get_os_info")
+    def test_new_rhel_derived(self, mock_get_os_info):
+        for os_info in [("centos", "9"), ("rhel", "9"), ("oracle", "9")]:
+            mock_get_os_info.return_value = os_info
+            config = util.get_apache_configurator(
+                self.config_path, self.vhost_path, self.config_dir, self.work_dir,
+                os_info=os_info[0])
+            self.assertEqual(config.options.ctl, "apachectl")
+            self.assertEqual(config.options.bin, "httpd")
+            self.assertEqual(config.options.version_cmd, ["httpd", "-v"])
+            self.assertEqual(config.options.restart_cmd, ["apachectl", "graceful"])
+            self.assertEqual(config.options.restart_cmd_alt, ["apachectl", "restart"])
+            self.assertEqual(config.options.conftest_cmd, ["apachectl", "configtest"])
+            self.assertEqual(config.options.get_defines_cmd, ["httpd", "-t", "-D", "DUMP_RUN_CFG"])
+            self.assertEqual(config.options.get_includes_cmd, ["httpd", "-t", "-D", "DUMP_INCLUDES"])
+            self.assertEqual(config.options.get_modules_cmd, ["httpd", "-t", "-D", "DUMP_MODULES"])
+
+
 class MultipleVhostsTestCentOS(util.ApacheTest):
     """Multiple vhost tests for CentOS / RHEL family of distros"""
 
     _multiprocess_can_split_ = True
 
-    def setUp(self):  # pylint: disable=arguments-differ
+    @mock.patch("certbot.util.get_os_info")
+    def setUp(self, mock_get_os_info):  # pylint: disable=arguments-differ
         test_dir = "centos7_apache/apache"
         config_root = "centos7_apache/apache/httpd"
         vhost_root = "centos7_apache/apache/httpd/conf.d"
         super().setUp(test_dir=test_dir,
                       config_root=config_root,
                       vhost_root=vhost_root)
-
+        mock_get_os_info.return_value = ("centos", "9")
         self.config = util.get_apache_configurator(
             self.config_path, self.vhost_path, self.config_dir, self.work_dir,
             os_info="centos")
@@ -124,9 +171,9 @@ class MultipleVhostsTestCentOS(util.ApacheTest):
         )
         def mock_get_cfg(command):
             """Mock httpd process stdout"""
-            if command == ['apachectl', '-t', '-D', 'DUMP_RUN_CFG']:
+            if command == ['httpd', '-t', '-D', 'DUMP_RUN_CFG']:
                 return define_val
-            elif command == ['apachectl', '-t', '-D', 'DUMP_MODULES']:
+            elif command == ['httpd', '-t', '-D', 'DUMP_MODULES']:
                 return mod_val
             return ""
         mock_get.side_effect = mock_get_cfg
@@ -135,7 +182,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest):
 
         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 = ("centos", "7")
+            mock_osi.return_value = ("centos", "9")
             self.config.parser.update_runtime_variables()
 
         self.assertEqual(mock_get.call_count, 3)
@@ -170,7 +217,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest):
 
         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 = ("centos", "7")
+            mock_osi.return_value = ("centos", "9")
             self.config.parser.update_runtime_variables()
 
         self.assertIn("mock_define", self.config.parser.variables)
diff --git a/certbot-apache/tests/util.py b/certbot-apache/tests/util.py
index a4191b3fe..2f119938b 100644
--- a/certbot-apache/tests/util.py
+++ b/certbot-apache/tests/util.py
@@ -123,6 +123,7 @@ def get_apache_configurator(
                         # Custom virtualhost path was requested
                         config.config.apache_vhost_root = conf_vhost_path
                     config.config.apache_ctl = config_class.OS_DEFAULTS.ctl
+                    config.config.apache_bin = config_class.OS_DEFAULTS.bin
                     config.prepare()
     return config
 
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 9403209dc..a6b6705a1 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -176,6 +176,8 @@ More details about these changes can be found on our GitHub repo.
 
 * Updated dependencies to use new version of cryptography that uses OpenSSL 1.1.1n, in
   response to https://www.openssl.org/news/secadv/20220315.txt.
+* CentOS 9 and other RHEL-derived OSes now correctly use httpd instead of apachectl for
+  various Apache-related commands
 
 More details about these changes can be found on our GitHub repo.