From 6148e5c35599cc3b39a5be70e4c520414d1e5eb7 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Tue, 3 Dec 2019 01:00:53 +0200 Subject: [PATCH] [Apache v2] Move the apachectl parsing to apache_util (#7569) * Move the Apache CLI parsing to apache_util * Fix test mocks * Address review comments * Fix the parsernode metadata dictionary --- certbot-apache/certbot_apache/apache_util.py | 119 ++++++++++++++++++ certbot-apache/certbot_apache/configurator.py | 7 ++ .../certbot_apache/override_gentoo.py | 2 +- certbot-apache/certbot_apache/parser.py | 77 +----------- .../certbot_apache/tests/centos_test.py | 4 +- .../certbot_apache/tests/configurator_test.py | 4 +- .../certbot_apache/tests/debian_test.py | 2 +- .../certbot_apache/tests/fedora_test.py | 4 +- .../certbot_apache/tests/gentoo_test.py | 4 +- .../certbot_apache/tests/parser_test.py | 12 +- certbot-apache/certbot_apache/tests/util.py | 28 +++-- 11 files changed, 163 insertions(+), 100 deletions(-) diff --git a/certbot-apache/certbot_apache/apache_util.py b/certbot-apache/certbot_apache/apache_util.py index 7a2ecf49b..70febc949 100644 --- a/certbot-apache/certbot_apache/apache_util.py +++ b/certbot-apache/certbot_apache/apache_util.py @@ -1,9 +1,16 @@ """ Utility functions for certbot-apache plugin """ import binascii +import logging +import re +import subprocess +from certbot import errors from certbot import util + from certbot.compat import os +logger = logging.getLogger(__name__) + def get_mod_deps(mod_name): """Get known module dependencies. @@ -105,3 +112,115 @@ def parse_define_file(filepath, varname): def unique_id(): """ Returns an unique id to be used as a VirtualHost identifier""" return binascii.hexlify(os.urandom(16)).decode("utf-8") + + +def parse_defines(apachectl): + """ + Gets Defines from httpd process and returns a dictionary of + the defined variables. + + :param str apachectl: Path to apachectl executable + + :returns: dictionary of defined variables + :rtype: dict + """ + + variables = dict() + define_cmd = [apachectl, "-t", "-D", + "DUMP_RUN_CFG"] + matches = parse_from_subprocess(define_cmd, r"Define: ([^ \n]*)") + try: + matches.remove("DUMP_RUN_CFG") + except ValueError: + return {} + + for match in matches: + if match.count("=") > 1: + logger.error("Unexpected number of equal signs in " + "runtime config dump.") + raise errors.PluginError( + "Error parsing Apache runtime variables") + parts = match.partition("=") + variables[parts[0]] = parts[2] + + return variables + + +def parse_includes(apachectl): + """ + Gets Include directives from httpd process and returns a list of + their values. + + :param str apachectl: Path to apachectl executable + + :returns: list of found Include directive values + :rtype: list of str + """ + + inc_cmd = [apachectl, "-t", "-D", + "DUMP_INCLUDES"] + return parse_from_subprocess(inc_cmd, r"\(.*\) (.*)") + + +def parse_modules(apachectl): + """ + Get loaded modules from httpd process, and return the list + of loaded module names. + + :param str apachectl: Path to apachectl executable + + :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") + + +def parse_from_subprocess(command, regexp): + """Get values from stdout of subprocess command + + :param list command: Command to run + :param str regexp: Regexp for parsing + + :returns: list parsed from command output + :rtype: list + + """ + stdout = _get_runtime_cfg(command) + return re.compile(regexp).findall(stdout) + + +def _get_runtime_cfg(command): + """ + Get runtime configuration info. + + :param command: Command to run + + :returns: stdout from command + + """ + try: + proc = subprocess.Popen( + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + universal_newlines=True) + stdout, stderr = proc.communicate() + + except (OSError, ValueError): + logger.error( + "Error running command %s for runtime parameters!%s", + command, os.linesep) + raise errors.MisconfigurationError( + "Error accessing loaded Apache parameters: {0}".format( + command)) + # Small errors that do not impede + if proc.returncode != 0: + logger.warning("Error in checking parameter list: %s", stderr) + raise errors.MisconfigurationError( + "Apache is unable to check whether or not the module is " + "loaded because Apache is misconfigured.") + + return stdout diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 2b20ebb7c..9a9dec7a8 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -359,6 +359,13 @@ class ApacheConfigurator(common.Installer): def get_parsernode_root(self, metadata): """Initializes the ParserNode parser root instance.""" + + apache_vars = dict() + apache_vars["defines"] = apache_util.parse_defines(self.option("ctl")) + apache_vars["includes"] = apache_util.parse_includes(self.option("ctl")) + apache_vars["modules"] = apache_util.parse_modules(self.option("ctl")) + metadata["apache_vars"] = apache_vars + return dualparser.DualBlockNode( name=assertions.PASS, ancestor=None, diff --git a/certbot-apache/certbot_apache/override_gentoo.py b/certbot-apache/certbot_apache/override_gentoo.py index c358a10fa..fdb1a8d8a 100644 --- a/certbot-apache/certbot_apache/override_gentoo.py +++ b/certbot-apache/certbot_apache/override_gentoo.py @@ -70,6 +70,6 @@ class GentooParser(parser.ApacheParser): def update_modules(self): """Get loaded modules from httpd process, and add them to DOM""" mod_cmd = [self.configurator.option("ctl"), "modules"] - matches = self.parse_from_subprocess(mod_cmd, r"(.*)_module") + matches = apache_util.parse_from_subprocess(mod_cmd, r"(.*)_module") for mod in matches: self.add_mod(mod.strip()) diff --git a/certbot-apache/certbot_apache/parser.py b/certbot-apache/certbot_apache/parser.py index 906b97e72..b9bdaca37 100644 --- a/certbot-apache/certbot_apache/parser.py +++ b/certbot-apache/certbot_apache/parser.py @@ -3,7 +3,6 @@ import copy import fnmatch import logging import re -import subprocess import sys import six @@ -13,6 +12,7 @@ from acme.magic_typing import Dict, List, Set # pylint: disable=unused-import, from certbot import errors from certbot.compat import os +from certbot_apache import apache_util from certbot_apache import constants logger = logging.getLogger(__name__) @@ -291,32 +291,15 @@ class ApacheParser(object): def update_runtime_variables(self): """Update Includes, Defines and Includes from httpd config dump data""" + self.update_defines() self.update_includes() self.update_modules() def update_defines(self): - """Get Defines from httpd process""" + """Updates the dictionary of known variables in the configuration""" - variables = dict() - define_cmd = [self.configurator.option("ctl"), "-t", "-D", - "DUMP_RUN_CFG"] - matches = self.parse_from_subprocess(define_cmd, r"Define: ([^ \n]*)") - try: - matches.remove("DUMP_RUN_CFG") - except ValueError: - return - - for match in matches: - if match.count("=") > 1: - logger.error("Unexpected number of equal signs in " - "runtime config dump.") - raise errors.PluginError( - "Error parsing Apache runtime variables") - parts = match.partition("=") - variables[parts[0]] = parts[2] - - self.variables = variables + self.variables = apache_util.parse_defines(self.configurator.option("ctl")) def update_includes(self): """Get includes from httpd process, and add them to DOM if needed""" @@ -326,9 +309,7 @@ class ApacheParser(object): # configuration files _ = self.find_dir("Include") - inc_cmd = [self.configurator.option("ctl"), "-t", "-D", - "DUMP_INCLUDES"] - matches = self.parse_from_subprocess(inc_cmd, r"\(.*\) (.*)") + matches = apache_util.parse_includes(self.configurator.option("ctl")) if matches: for i in matches: if not self.parsed_in_current(i): @@ -337,56 +318,10 @@ class ApacheParser(object): def update_modules(self): """Get loaded modules from httpd process, and add them to DOM""" - mod_cmd = [self.configurator.option("ctl"), "-t", "-D", - "DUMP_MODULES"] - matches = self.parse_from_subprocess(mod_cmd, r"(.*)_module") + matches = apache_util.parse_modules(self.configurator.option("ctl")) for mod in matches: self.add_mod(mod.strip()) - def parse_from_subprocess(self, command, regexp): - """Get values from stdout of subprocess command - - :param list command: Command to run - :param str regexp: Regexp for parsing - - :returns: list parsed from command output - :rtype: list - - """ - stdout = self._get_runtime_cfg(command) - return re.compile(regexp).findall(stdout) - - def _get_runtime_cfg(self, command): # pylint: disable=no-self-use - """Get runtime configuration info. - :param command: Command to run - - :returns: stdout from command - - """ - try: - proc = subprocess.Popen( - command, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - universal_newlines=True) - stdout, stderr = proc.communicate() - - except (OSError, ValueError): - logger.error( - "Error running command %s for runtime parameters!%s", - command, os.linesep) - raise errors.MisconfigurationError( - "Error accessing loaded Apache parameters: {0}".format( - command)) - # Small errors that do not impede - if proc.returncode != 0: - logger.warning("Error in checking parameter list: %s", stderr) - raise errors.MisconfigurationError( - "Apache is unable to check whether or not the module is " - "loaded because Apache is misconfigured.") - - return stdout - def filter_args_num(self, matches, args): # pylint: disable=no-self-use """Filter out directives with specific number of arguments. diff --git a/certbot-apache/certbot_apache/tests/centos_test.py b/certbot-apache/certbot_apache/tests/centos_test.py index dddbf489e..bb97ec144 100644 --- a/certbot-apache/certbot_apache/tests/centos_test.py +++ b/certbot-apache/certbot_apache/tests/centos_test.py @@ -107,7 +107,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest): def test_get_parser(self): self.assertIsInstance(self.config.parser, override_centos.CentOSParser) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): define_val = ( 'Define: TEST1\n' @@ -156,7 +156,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest): raise Exception("Missed: %s" % vhost) # pragma: no cover self.assertEqual(found, 2) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._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 diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 68d9c90fa..350112f6f 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -807,7 +807,7 @@ class MultipleVhostsTest(util.ApacheTest): self.assertEqual(mock_restart.call_count, 1) @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart") - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_cleanup(self, mock_cfg, mock_restart): mock_cfg.return_value = "" _, achalls = self.get_key_and_achalls() @@ -823,7 +823,7 @@ class MultipleVhostsTest(util.ApacheTest): self.assertFalse(mock_restart.called) @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart") - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_cleanup_no_errors(self, mock_cfg, mock_restart): mock_cfg.return_value = "" _, achalls = self.get_key_and_achalls() diff --git a/certbot-apache/certbot_apache/tests/debian_test.py b/certbot-apache/certbot_apache/tests/debian_test.py index 54ced2d0b..72f806589 100644 --- a/certbot-apache/certbot_apache/tests/debian_test.py +++ b/certbot-apache/certbot_apache/tests/debian_test.py @@ -47,7 +47,7 @@ class MultipleVhostsTestDebian(util.ApacheTest): @mock.patch("certbot.util.run_script") @mock.patch("certbot.util.exe_exists") - @mock.patch("certbot_apache.parser.subprocess.Popen") + @mock.patch("certbot_apache.apache_util.subprocess.Popen") def test_enable_mod(self, mock_popen, mock_exe_exists, mock_run_script): mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "") mock_popen().returncode = 0 diff --git a/certbot-apache/certbot_apache/tests/fedora_test.py b/certbot-apache/certbot_apache/tests/fedora_test.py index 4d3f3a313..799c24c20 100644 --- a/certbot-apache/certbot_apache/tests/fedora_test.py +++ b/certbot-apache/certbot_apache/tests/fedora_test.py @@ -101,7 +101,7 @@ class MultipleVhostsTestFedora(util.ApacheTest): def test_get_parser(self): self.assertIsInstance(self.config.parser, override_fedora.FedoraParser) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): define_val = ( 'Define: TEST1\n' @@ -156,7 +156,7 @@ class MultipleVhostsTestFedora(util.ApacheTest): raise Exception("Missed: %s" % vhost) # pragma: no cover self.assertEqual(found, 2) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._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 diff --git a/certbot-apache/certbot_apache/tests/gentoo_test.py b/certbot-apache/certbot_apache/tests/gentoo_test.py index d0d3ba0dd..353a66564 100644 --- a/certbot-apache/certbot_apache/tests/gentoo_test.py +++ b/certbot-apache/certbot_apache/tests/gentoo_test.py @@ -90,7 +90,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest): for define in defines: self.assertTrue(define in self.config.parser.variables.keys()) - @mock.patch("certbot_apache.parser.ApacheParser.parse_from_subprocess") + @mock.patch("certbot_apache.apache_util.parse_from_subprocess") def test_no_binary_configdump(self, mock_subprocess): """Make sure we don't call binary dumps other than modules from Apache as this is not supported in Gentoo currently""" @@ -104,7 +104,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest): self.config.parser.reset_modules() self.assertTrue(mock_subprocess.called) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): mod_val = ( 'Loaded Modules:\n' diff --git a/certbot-apache/certbot_apache/tests/parser_test.py b/certbot-apache/certbot_apache/tests/parser_test.py index 27d66f680..303ac09e3 100644 --- a/certbot-apache/certbot_apache/tests/parser_test.py +++ b/certbot-apache/certbot_apache/tests/parser_test.py @@ -167,7 +167,7 @@ class BasicParserTest(util.ParserTest): self.assertTrue(mock_logger.debug.called) @mock.patch("certbot_apache.parser.ApacheParser.find_dir") - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_update_runtime_variables(self, mock_cfg, _): define_val = ( 'ServerRoot: "/etc/apache2"\n' @@ -273,7 +273,7 @@ class BasicParserTest(util.ParserTest): self.assertEqual(mock_parse.call_count, 25) @mock.patch("certbot_apache.parser.ApacheParser.find_dir") - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_update_runtime_variables_alt_values(self, mock_cfg, _): inc_val = ( 'Included configuration files:\n' @@ -295,7 +295,7 @@ class BasicParserTest(util.ParserTest): # path derived from root configuration Include statements self.assertEqual(mock_parse.call_count, 1) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_update_runtime_vars_bad_output(self, mock_cfg): mock_cfg.return_value = "Define: TLS=443=24" self.parser.update_runtime_variables() @@ -305,7 +305,7 @@ class BasicParserTest(util.ParserTest): errors.PluginError, self.parser.update_runtime_variables) @mock.patch("certbot_apache.configurator.ApacheConfigurator.option") - @mock.patch("certbot_apache.parser.subprocess.Popen") + @mock.patch("certbot_apache.apache_util.subprocess.Popen") def test_update_runtime_vars_bad_ctl(self, mock_popen, mock_opt): mock_popen.side_effect = OSError mock_opt.return_value = "nonexistent" @@ -313,7 +313,7 @@ class BasicParserTest(util.ParserTest): errors.MisconfigurationError, self.parser.update_runtime_variables) - @mock.patch("certbot_apache.parser.subprocess.Popen") + @mock.patch("certbot_apache.apache_util.subprocess.Popen") def test_update_runtime_vars_bad_exit(self, mock_popen): mock_popen().communicate.return_value = ("", "") mock_popen.returncode = -1 @@ -357,7 +357,7 @@ class ParserInitTest(util.ApacheTest): ApacheParser, os.path.relpath(self.config_path), "/dummy/vhostpath", version=(2, 4, 22), configurator=self.config) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_unparseable(self, mock_cfg): from certbot_apache.parser import ApacheParser mock_cfg.return_value = ('Define: TEST') diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py index 8e3de04be..4155c93c0 100644 --- a/certbot-apache/certbot_apache/tests/util.py +++ b/certbot-apache/certbot_apache/tests/util.py @@ -111,19 +111,21 @@ def get_apache_configurator( # pylint: disable=too-many-arguments, too-many-loc mock_exe_exists.return_value = True with mock.patch("certbot_apache.parser.ApacheParser." "update_runtime_variables"): - try: - config_class = entrypoint.OVERRIDE_CLASSES[os_info] - except KeyError: - config_class = configurator.ApacheConfigurator - config = config_class(config=mock_le_config, name="apache", - version=version) - if not conf_vhost_path: - config_class.OS_DEFAULTS["vhost_root"] = vhost_path - else: - # Custom virtualhost path was requested - config.config.apache_vhost_root = conf_vhost_path - config.config.apache_ctl = config_class.OS_DEFAULTS["ctl"] - config.prepare() + with mock.patch("certbot_apache.apache_util.parse_from_subprocess") as mock_sp: + mock_sp.return_value = [] + try: + config_class = entrypoint.OVERRIDE_CLASSES[os_info] + except KeyError: + config_class = configurator.ApacheConfigurator + config = config_class(config=mock_le_config, name="apache", + version=version) + if not conf_vhost_path: + config_class.OS_DEFAULTS["vhost_root"] = vhost_path + else: + # Custom virtualhost path was requested + config.config.apache_vhost_root = conf_vhost_path + config.config.apache_ctl = config_class.OS_DEFAULTS["ctl"] + config.prepare() return config