Cleanup Apache

letsencrypt-apache/letsencrypt_apache/configurator.py

@@ -1,4 +1,5 @@
 """Apache Configuration based off of Augeas Configurator."""
+# pylint: disable=too-many-lines
 import logging
 import os
 import re
@@ -16,8 +17,6 @@ from letsencrypt import errors
 from letsencrypt import interfaces
 from letsencrypt import le_util
 
-from letsencrypt.plugins import common
-
 from letsencrypt_apache import augeas_configurator
 from letsencrypt_apache import constants
 from letsencrypt_apache import display_ops
@@ -168,17 +167,21 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         """
         vhost = self.choose_vhost(domain)
 
+        # This is done first so that ssl module is enabled and cert_path,
+        # cert_key... can all be parsed appropriately
+        self.prepare_server_https("443")
+
         path = {}
 
-        path["cert_path"] = self.parser.find_dir(parser.case_i(
-            "SSLCertificateFile"), None, vhost.path)
-        path["cert_key"] = self.parser.find_dir(parser.case_i(
-            "SSLCertificateKeyFile"), None, vhost.path)
+        path["cert_path"] = self.parser.find_dir(
+            "SSLCertificateFile", None, vhost.path)
+        path["cert_key"] = self.parser.find_dir(
+            "SSLCertificateKeyFile", None, vhost.path)
 
         # Only include if a certificate chain is specified
         if chain_path is not None:
             path["chain_path"] = self.parser.find_dir(
-                parser.case_i("SSLCertificateChainFile"), None, vhost.path)
+                "SSLCertificateChainFile", None, vhost.path)
 
         if not path["cert_path"] or not path["cert_key"]:
             # Throw some can't find all of the directives error"
@@ -186,7 +189,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 "Cannot find a cert or key directive in %s. "
                 "VirtualHost was not modified", vhost.path)
             # Presumably break here so that the virtualhost is not modified
-            return False
+            raise errors.PluginError(
+                "Unable to find cert and/or key directives")
 
         logger.info("Deploying Certificate to VirtualHost %s", vhost.filep)
 
@@ -235,7 +239,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         vhost = self._find_best_vhost(target_name)
         if vhost is not None:
             if not vhost.ssl:
-                vhost = self.make_vhost_ssl(non_ssl_vhost)
+                vhost = self.make_vhost_ssl(vhost)
 
             self.assoc[target_name] = vhost
             return vhost
@@ -311,13 +315,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         for vhost in self.vhosts:
             all_names.update(vhost.names)
             for addr in vhost.addrs:
-                name = get_name_from_ip(addr)
+                name = self.get_name_from_ip(addr)
                 if name:
                     all_names.add(name)
 
         return all_names
 
-    def get_name_from_ip(self, addr):
+    def get_name_from_ip(self, addr):  # pylint: disable=no-self-use
         """Returns a reverse dns name if available.
 
         :param addr: IP Address
@@ -328,7 +332,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         """
         # If it isn't a private IP, do a reverse DNS lookup
-        if not private_ips_regex.match(addr.get_addr()):
+        if not ApacheConfigurator.private_ips_regex.match(addr.get_addr()):
             try:
                 socket.inet_aton(addr.get_addr())
                 return socket.gethostbyaddr(addr.get_addr())[0]
@@ -371,12 +375,12 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             addrs.add(obj.Addr.fromstring(self.parser.get_arg(arg)))
         is_ssl = False
 
-        if self.parser.find_dir(
-                parser.case_i("SSLEngine"), parser.case_i("on"), path):
+        if self.parser.find_dir("SSLEngine", "on", path, exclude=False):
             is_ssl = True
 
         filename = get_file_path(path)
         is_enabled = self.is_site_enabled(filename)
+
         vhost = obj.VirtualHost(filename, path, addrs, is_ssl, is_enabled)
         self._add_servernames(vhost)
         return vhost
@@ -394,6 +398,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         paths = self.aug.match(
             ("/files%s/sites-available//*[label()=~regexp('%s')]" %
              (self.parser.root, parser.case_i("VirtualHost"))))
+
         vhs = []
 
         for path in paths:
@@ -402,7 +407,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         return vhs
 
     def is_name_vhost(self, target_addr):
-        r"""Returns if vhost is a name based vhost
+        """Returns if vhost is a name based vhost
 
         NameVirtualHost was deprecated in Apache 2.4 as all VirtualHosts are
         now NameVirtualHosts. If version is earlier than 2.4, check if addr
@@ -421,9 +426,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # search for NameVirtualHost directive for ip_addr
         # note ip_addr can be FQDN although Apache does not recommend it
         return (self.version >= (2, 4) or
-                self.parser.find_dir(
-                    parser.case_i("NameVirtualHost"),
-                    parser.case_i(str(target_addr))))
+                self.parser.find_dir("NameVirtualHost", str(target_addr)))
 
     def add_name_vhost(self, addr):
         """Adds NameVirtualHost directive for given address.
@@ -432,14 +435,17 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         :type addr: :class:~letsencrypt_apache.obj.Addr
 
         """
-        path = self.parser.add_dir_to_ifmodssl(
-            parser.get_aug_path(
-                self.parser.loc["name"]), "NameVirtualHost", [str(addr)])
+        loc = parser.get_aug_path(self.parser.loc["name"])
+        if addr.get_port == "443":
+            path = self.parser.add_dir_to_ifmodssl(
+                loc, "NameVirtualHost", [str(addr)])
+        else:
+            path = self.parser.add_dir(loc, "NameVirtualHost", [str(addr)])
 
         self.save_notes += "Setting %s to be NameBasedVirtualHost\n" % addr
         self.save_notes += "\tDirective added to %s\n" % path
 
-    def _prepare_server_https(self, port):
+    def prepare_server_https(self, port):
         """Prepare the server for HTTPS.
 
         Make sure that the ssl_module is loaded and that the server
@@ -454,9 +460,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         # Check for Listen <port>
         # Note: This could be made to also look for ip:443 combo
-        if not self.parser.find_dir(parser.case_i("Listen"), port):
-            logger.debug("No Listen {0} directive found. Setting the "
-                         "Apache Server to Listen on port {0}".format(port))
+        if not self.parser.find_dir("Listen", port):
+            logger.debug("No Listen %s directive found. Setting the "
+                         "Apache Server to Listen on port %s", port, port)
 
             if port == "443":
                 args = [port]
@@ -540,7 +546,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             vh_p = vh_p[0]
 
         # Update Addresses
-        ssl_addrs = self._update_ssl_vhosts_addrs(vh_p)
+        self._update_ssl_vhosts_addrs(vh_p)
 
         # Add directives
         self._add_dummy_ssl_directives(vh_p)
@@ -552,7 +558,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         # We know the length is one because of the assertion above
         # Create the Vhost object
-        ssl_vhost = self._create_vhost(vh_p[0])
+        ssl_vhost = self._create_vhost(vh_p)
         self.vhosts.append(ssl_vhost)
 
 
@@ -632,7 +638,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         # See if the exact address appears in any other vhost
         for addr in vhost.addrs:
-            for test_vhost in self.vhosts:
+            for test_vh in self.vhosts:
                 if (vhost.filep != test_vh.filep and addr in test_vh.addrs and
                         not self.is_name_vhost(addr)):
                     self.add_name_vhost(addr)
@@ -746,10 +752,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         :rtype: bool, int
 
         """
-        rewrite_path = self.parser.find_dir(
-            parser.case_i("RewriteRule"), None, vhost.path)
-        redirect_path = self.parser.find_dir(
-            parser.case_i("Redirect"), None, vhost.path)
+        rewrite_path = self.parser.find_dir("RewriteRule", None, vhost.path)
+        redirect_path = self.parser.find_dir("Redirect", None, vhost.path)
 
         if redirect_path:
             # "Existing Redirect directive for virtualhost"
@@ -942,9 +946,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         for vhost in self.vhosts:
             if vhost.ssl:
                 cert_path = self.parser.find_dir(
-                    parser.case_i("SSLCertificateFile"), None, vhost.path)
+                    "SSLCertificateFile", None, vhost.path)
                 key_path = self.parser.find_dir(
-                    parser.case_i("SSLCertificateKeyFile"), None, vhost.path)
+                    "SSLCertificateKeyFile", None, vhost.path)
 
                 # Can be removed once find directive can return ordered results
                 if len(cert_path) != 1 or len(key_path) != 1:
@@ -981,7 +985,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         """Enables an available site, Apache restart required.
 
         .. todo:: This function should number subdomains before the domain vhost
-
         .. todo:: Make sure link is not broken...
 
         :param vhost: vhost to enable
@@ -996,9 +999,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         if vhost.ssl:
             # TODO: Make this based on addresses
-            self._prepare_server_https("443")
+            self.prepare_server_https("443")
             if self.save_notes:
-                self.save("Enabled TLS for Apache")
+                self.save()
 
         if "/sites-available/" in vhost.filep:
             enabled_path = ("%s/sites-enabled/%s" %
@@ -1034,20 +1037,21 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # Modules can enable additional config files. Variables may be defined
         # within these new configuration sections.
         # Restart is not necessary as DUMP_RUN_CFG uses latest config.
-        self.parser.update_runtime_variables()
+        self.parser.update_runtime_variables(self.conf("ctl"))
 
         self.parser.modules.add(mod_name + "_module")
-        self.parser.modules.add("mod_" + mod_name)
+        self.parser.modules.add("mod_" + mod_name + ".c")
 
     def _enable_mod_debian(self, mod_name):
         """Assumes mods-available, mods-enabled layout."""
         # TODO: This can be further updated to not require all files.
         if mod_name == "ssl":
-            self._enable_mod_debian_files(["ssl.conf", "ssl.load"])
+            self._enable_mod_debian_files(
+                ["ssl.conf", "ssl.load"], "ssl_module")
         elif mod_name == "rewrite":
-            self._enable_mod_debian_files(["rewrite.load"])
+            self._enable_mod_debian_files(["rewrite.load"], "rewrite_module")
         else:
-            raise NotImplemented
+            raise NotImplementedError
 
     def _enable_mod_debian_files(self, filenames, mod_name):
         """Move over all required files into mods-enabled."""
@@ -1058,11 +1062,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         for filename in filenames:
             if not os.path.isfile(os.path.join(mods_available, filename)):
                 raise errors.MisconfigurationError(
-            "Unable to enable module. Required files missing from "
-            "mods-available. %s" % str(filenames))
+                    "Unable to enable module. Required files missing from "
+                    "mods-available. %s" % str(filenames))
 
         # Register and symlink files
-        for filename in files:
+        for filename in filenames:
             enabled_path = os.path.join(mods_enabled, filename)
             if os.path.isfile(enabled_path):
                 logger.debug(
@@ -1131,6 +1135,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             raise errors.PluginError("Unable to run apache2ctl")
 
         if proc.returncode != 0:
+            print proc.returncode
             # Enter recovery routine...
             logger.error("Apache Configtest failed\n%s\n%s", stdout, stderr)
             raise errors.MisconfigurationError(

letsencrypt-apache/letsencrypt_apache/dvsni.py

@@ -45,7 +45,7 @@ class ApacheDvsni(common.Dvsni):
 
 """
 
-    def __init__(self):
+    def __init__(self, *args, **kwargs):
         super(ApacheDvsni, self).__init__(*args, **kwargs)
 
         self.challenge_conf = os.path.join(
@@ -60,6 +60,10 @@ class ApacheDvsni(common.Dvsni):
         # About to make temporary changes to the config
         self.configurator.save()
 
+        # Prepare the server for HTTPS
+        self.configurator.prepare_server_https(
+            str(self.configurator.config.dvsni_port))
+
         responses = []
 
         # Create all of the challenge certs
@@ -68,13 +72,8 @@ class ApacheDvsni(common.Dvsni):
 
         # Setup the configuration
         dvsni_addrs = self._mod_config()
-
         self.configurator.make_addrs_sni_ready(dvsni_addrs)
 
-        # Prepare the server for HTTPS
-        self.configurator._prepare_https_server(
-            str(self.configurator.config.dvsni_port))
-
         # Save reversible changes
         self.configurator.save("SNI Challenge", True)
 
@@ -96,7 +95,7 @@ class ApacheDvsni(common.Dvsni):
             achall_addrs = self.get_dvsni_addrs(achall)
             dvsni_addrs.update(achall_addrs)
 
-            config_text += self._get_config_text(self.achalls, achall_addrs)
+            config_text += self._get_config_text(achall, achall_addrs)
 
         config_text += "</IfModule>\n"
 
@@ -114,7 +113,6 @@ class ApacheDvsni(common.Dvsni):
         vhost = self.configurator.choose_vhost(achall.domain)
 
         # TODO: Checkout _default_ rules.
-        # TODO: Need to separate out test mode and normal mode for DVSNI addrs
         dvsni_addrs = set()
         default_addr = obj.Addr(("*", self.configurator.config.dvsni_port))
 

letsencrypt-apache/letsencrypt_apache/obj.py

@@ -2,7 +2,7 @@
 from letsencrypt.plugins import common
 
 class Addr(common.Addr):
-
+    """Represents an Apache address."""
     def __eq__(self, other):
         """This is defined as equalivalent within Apache.
 
@@ -16,7 +16,8 @@ class Addr(common.Addr):
         return False
 
     def is_wildcard(self):
-        return tup[1] == "*" or not tup[1]
+        """Returns if address has a wildcard port."""
+        return self.tup[1] == "*" or not self.tup[1]
 
     def get_sni_addr(self, port):
         """Returns the least specific address that resolves on the port.

letsencrypt-apache/letsencrypt_apache/parser.py

@@ -1,5 +1,4 @@
 """ApacheParser is a member object of the ApacheConfigurator class."""
-import collections
 import fnmatch
 import itertools
 import logging
@@ -13,10 +12,6 @@ from letsencrypt import errors
 logger = logging.getLogger(__name__)
 
 
-arg_var_interpreter = re.compile(r"\$\{[^ \}]*}")
-fnmatch_chars = set(["*", "?", "\\", "[", "]"])
-
-
 class ApacheParser(object):
     """Class handles the fine details of parsing the Apache Configuration.
 
@@ -26,6 +21,9 @@ class ApacheParser(object):
         directory. Without trailing slash.
 
     """
+    arg_var_interpreter = re.compile(r"\$\{[^ \}]*}")
+    fnmatch_chars = set(["*", "?", "\\", "[", "]"])
+
     def __init__(self, aug, root, ssl_options, ctl):
         # This uses the binary, so it can be done first.
         # https://httpd.apache.org/docs/2.4/mod/core.html#define
@@ -59,7 +57,7 @@ class ApacheParser(object):
             the iteration issue.  Else... parse and enable mods at same time.
 
         """
-        matches = self.find_dir(case_i("LoadModule"))
+        matches = self.find_dir("LoadModule")
 
         iterator = iter(matches)
         # Make sure prev_size != cur_size for do: while: iteration
@@ -101,7 +99,7 @@ class ApacheParser(object):
 
         self.variables = variables
 
-    def _get_runtime_cfg(self, ctl):
+    def _get_runtime_cfg(self, ctl):  # pylint: disable=no-self-use
         """Get runtime configuration info.
 
         :returns: stdout from DUMP_RUN_CFG
@@ -116,8 +114,7 @@ class ApacheParser(object):
 
         except (OSError, ValueError):
             logger.error(
-                "Error accessing {0} for runtime parameters!{1}".format(
-                    ctl, os.linesep))
+                "Error accessing %s for runtime parameters!%s", ctl, os.linesep)
             raise errors.MisconfigurationError(
                 "Error accessing loaded Apache parameters: %s", ctl)
         # Small errors that do not impede
@@ -129,7 +126,7 @@ class ApacheParser(object):
 
         return stdout
 
-    def _filter_args_num(self, matches, args):
+    def _filter_args_num(self, matches, args):  # pylint: disable=no-self-use
         """Filter out directives with specific number of arguments.
 
         This function makes the assumption that all related arguments are given
@@ -223,7 +220,7 @@ class ApacheParser(object):
         else:
             self.aug.set(aug_conf_path + "/directive[last()]/arg", args)
 
-    def find_dir(self, directive, arg=None, start=None):
+    def find_dir(self, directive, arg=None, start=None, exclude=True):
         """Finds directive in the configuration.
 
         Recursively searches through config files to find directives
@@ -241,12 +238,13 @@ class ApacheParser(object):
         compatibility.
 
         :param str directive: Directive to look for
-
         :param arg: Specific value directive must have, None if all should
                     be considered
         :type arg: str or None
 
         :param str start: Beginning Augeas path to begin looking
+        :param bool exclude: Whether or not to exclude directives based on
+            variables and enabled modules
 
         """
         # Cannot place member variable in the definition of the function so...
@@ -265,32 +263,33 @@ class ApacheParser(object):
         # includes = self.aug.match(start +
         # "//* [self::directive='Include']/* [label()='arg']")
 
-        regex = "(%s)|(%s)|(%s)" % (directive,
+        regex = "(%s)|(%s)|(%s)" % (case_i(directive),
                                     case_i("Include"),
                                     case_i("IncludeOptional"))
         matches = self.aug.match(
             "%s//*[self::directive=~regexp('%s')]" % (start, regex))
 
-        matches = self._exclude_dirs(matches)
-
+        if exclude:
+            matches = self._exclude_dirs(matches)
 
         if arg is None:
             arg_suffix = "/arg"
         else:
-            arg_suffix = "/*[self::arg=~regexp('%s')]" % arg
+            arg_suffix = "/*[self::arg=~regexp('%s')]" % case_i(arg)
 
         ordered_matches = []
 
         # TODO: Wildcards should be included in alphabetical order
         # https://httpd.apache.org/docs/2.4/mod/core.html#include
         for match in matches:
-            dir = self.aug.get(match).lower()
-            if dir == "include" or dir == "includeoptional":
+            dir_ = self.aug.get(match).lower()
+            if dir_ == "include" or dir_ == "includeoptional":
                 # start[6:] to strip off /files
                 ordered_matches.extend(self.find_dir(
                     directive, arg, self._get_include_path(
                         self.get_arg(match + "/arg"))))
-            else:
+            # This additionally allows Include
+            if dir_ == directive.lower():
                 ordered_matches.extend(self.aug.match(match + arg_suffix))
 
         return ordered_matches
@@ -302,11 +301,14 @@ class ApacheParser(object):
 
         """
         value = self.aug.get(match)
-        variables = arg_var_interpreter.findall(value)
+        variables = ApacheParser.arg_var_interpreter.findall(value)
 
         for var in variables:
             # Strip off ${ and }
-            value = value.replace(var, self.variables[var[2:-1]])
+            try:
+                value = value.replace(var, self.variables[var[2:-1]])
+            except KeyError:
+                raise errors.PluginError("Error Parsing variable: %s" % var)
 
         return value
 
@@ -317,14 +319,14 @@ class ApacheParser(object):
         valid_matches = []
 
         for match in matches:
-            for filter in filters:
-                if not self._pass_filter(match, filter):
+            for filter_ in filters:
+                if not self._pass_filter(match, filter_):
                     break
             else:
                 valid_matches.append(match)
         return valid_matches
 
-    def _pass_filter(self, match, filter):
+    def _pass_filter(self, match, filter_):
         """Determine if directive passes a filter.
 
         :param str match: Augeas path
@@ -333,7 +335,7 @@ class ApacheParser(object):
 
         """
         match_l = match.lower()
-        last_match_idx = match_l.find(filter[0])
+        last_match_idx = match_l.find(filter_[0])
 
         while last_match_idx != -1:
             # Check args
@@ -341,10 +343,10 @@ class ApacheParser(object):
             expression = self.aug.get(match[:end_of_if] + "/arg")
 
             expected = not expression.startswith("!")
-            if expected != expression in filter[1]:
+            if expected != (expression in filter_[1]):
                 return False
 
-            last_match_idx = match_l.find(filter[0], end_of_if)
+            last_match_idx = match_l.find(filter_[0], end_of_if)
 
         return True
 
@@ -382,7 +384,7 @@ class ApacheParser(object):
         # then reassemble
         split_arg = arg.split("/")
         for idx, split in enumerate(split_arg):
-            if any(char in fnmatch_chars for char in split):
+            if any(char in ApacheParser.fnmatch_chars for char in split):
                 # Turn it into a augeas regex
                 # TODO: Can this instead be an augeas glob instead of regex
                 split_arg[idx] = ("* [label()=~regexp('%s')]" %
@@ -529,8 +531,7 @@ class ApacheParser(object):
         # in hierarchy via direct include
         # httpd.conf was very common as a user file in Apache 2.2
         if (os.path.isfile(os.path.join(self.root, "httpd.conf")) and
-                self.find_dir(
-                    case_i("Include"), case_i("httpd.conf"), root)):
+                self.find_dir("Include", "httpd.conf", root)):
             return os.path.join(self.root, "httpd.conf")
         else:
             return os.path.join(self.root, "apache2.conf")

letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py

@@ -1,6 +1,5 @@
 """Test for letsencrypt_apache.configurator."""
 import os
-import re
 import shutil
 import unittest
 
@@ -12,18 +11,16 @@ from letsencrypt import achallenges
 from letsencrypt import errors
 from letsencrypt import le_util
 
-from letsencrypt.plugins import common
-
 from letsencrypt.tests import acme_util
 
 from letsencrypt_apache import configurator
-from letsencrypt_apache import parser
+from letsencrypt_apache import obj
 
 from letsencrypt_apache.tests import util
 
 
 class TwoVhost80Test(util.ApacheTest):
-    """Test two standard well configured HTTP vhosts."""
+    """Test two standard well-configured HTTP vhosts."""
 
     def setUp(self):
         super(TwoVhost80Test, self).setUp()
@@ -48,7 +45,7 @@ class TwoVhost80Test(util.ApacheTest):
         """Make sure all vhosts are being properly found.
 
         .. note:: If test fails, only finding 1 Vhost... it is likely that
-            it is a problem with is_enabled.
+            it is a problem with is_enabled.  If finding only 3, likely is_ssl
 
         """
         vhs = self.config.get_virtual_hosts()
@@ -60,6 +57,8 @@ class TwoVhost80Test(util.ApacheTest):
                 if vhost == truth:
                     found += 1
                     break
+            else:
+                raise Exception("Missed: %s" % vhost)
 
         self.assertEqual(found, 4)
 
@@ -77,7 +76,35 @@ class TwoVhost80Test(util.ApacheTest):
         self.assertTrue(self.config.is_site_enabled(self.vh_truth[2].filep))
         self.assertTrue(self.config.is_site_enabled(self.vh_truth[3].filep))
 
-    def test_deploy_cert(self):
+    @mock.patch("letsencrypt_apache.parser.subprocess.Popen")
+    def test_enable_mod(self, mock_popen):
+        mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "")
+        mock_popen().returncode = 0
+
+        self.config.enable_mod("ssl")
+        for filename in ["ssl.conf", "ssl.load"]:
+            self.assertTrue(
+                os.path.isfile(os.path.join(
+                    self.config.conf("server-root"), "mods-enabled", filename)))
+
+        self.assertTrue("ssl_module" in self.config.parser.modules)
+        self.assertTrue("mod_ssl.c" in self.config.parser.modules)
+
+    @mock.patch("letsencrypt_apache.parser.subprocess.Popen")
+    def test_enable_site(self, mock_popen):
+        mock_popen().returncode = 0
+        mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "")
+
+        # Default 443 vhost
+        self.assertFalse(self.vh_truth[1].enabled)
+        self.config.enable_site(self.vh_truth[1])
+        self.assertTrue(self.vh_truth[1].enabled)
+
+    @mock.patch("letsencrypt_apache.parser.subprocess.Popen")
+    def test_deploy_cert(self, mock_popen):
+        mock_popen().returncode = 0
+        mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "")
+
         # Get the default 443 vhost
         self.config.assoc["random.demo"] = self.vh_truth[1]
         self.config.deploy_cert(
@@ -85,15 +112,17 @@ class TwoVhost80Test(util.ApacheTest):
             "example/cert.pem", "example/key.pem", "example/cert_chain.pem")
         self.config.save()
 
+        # Verify ssl_module was enabled.
+        self.assertTrue(self.vh_truth[1].enabled)
+        self.assertTrue("ssl_module" in self.config.parser.modules)
+
         loc_cert = self.config.parser.find_dir(
-            parser.case_i("sslcertificatefile"),
-            re.escape("example/cert.pem"), self.vh_truth[1].path)
+            "sslcertificatefile", "example/cert.pem", self.vh_truth[1].path)
         loc_key = self.config.parser.find_dir(
-            parser.case_i("sslcertificateKeyfile"),
-            re.escape("example/key.pem"), self.vh_truth[1].path)
+            "sslcertificateKeyfile", "example/key.pem", self.vh_truth[1].path)
         loc_chain = self.config.parser.find_dir(
-            parser.case_i("SSLCertificateChainFile"),
-            re.escape("example/cert_chain.pem"), self.vh_truth[1].path)
+            "SSLCertificateChainFile", "example/cert_chain.pem",
+            self.vh_truth[1].path)
 
         # Verify one directive was found in the correct file
         self.assertEqual(len(loc_cert), 1)
@@ -109,15 +138,15 @@ class TwoVhost80Test(util.ApacheTest):
                          self.vh_truth[1].filep)
 
     def test_is_name_vhost(self):
-        addr = common.Addr.fromstring("*:80")
+        addr = obj.Addr.fromstring("*:80")
         self.assertTrue(self.config.is_name_vhost(addr))
         self.config.version = (2, 2)
         self.assertFalse(self.config.is_name_vhost(addr))
 
     def test_add_name_vhost(self):
-        self.config.add_name_vhost("*:443")
+        self.config.add_name_vhost(obj.Addr.fromstring("*:443"))
         self.assertTrue(self.config.parser.find_dir(
-            "NameVirtualHost", re.escape("*:443")))
+            "NameVirtualHost", "*:443"))
 
     def test_make_vhost_ssl(self):
         ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0])
@@ -130,17 +159,15 @@ class TwoVhost80Test(util.ApacheTest):
         self.assertEqual(ssl_vhost.path,
                          "/files" + ssl_vhost.filep + "/IfModule/VirtualHost")
         self.assertEqual(len(ssl_vhost.addrs), 1)
-        self.assertEqual(set([common.Addr.fromstring("*:443")]), ssl_vhost.addrs)
+        self.assertEqual(set([obj.Addr.fromstring("*:443")]), ssl_vhost.addrs)
         self.assertEqual(ssl_vhost.names, set(["encryption-example.demo"]))
         self.assertTrue(ssl_vhost.ssl)
         self.assertFalse(ssl_vhost.enabled)
 
         self.assertTrue(self.config.parser.find_dir(
-            "SSLCertificateFile", None, ssl_vhost.path))
+            "SSLCertificateFile", None, ssl_vhost.path, False))
         self.assertTrue(self.config.parser.find_dir(
-            "SSLCertificateKeyFile", None, ssl_vhost.path))
-        self.assertTrue(self.config.parser.find_dir(
-            "Include", self.ssl_options, ssl_vhost.path))
+            "SSLCertificateKeyFile", None, ssl_vhost.path, False))
 
         self.assertEqual(self.config.is_name_vhost(self.vh_truth[0]),
                          self.config.is_name_vhost(ssl_vhost))

letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py

@@ -6,9 +6,9 @@ import mock
 
 from acme import challenges
 
-from letsencrypt.plugins import common
 from letsencrypt.plugins import common_test
 
+from letsencrypt_apache import obj
 from letsencrypt_apache.tests import util
 
 
@@ -20,11 +20,9 @@ class DvsniPerformTest(util.ApacheTest):
     def setUp(self):
         super(DvsniPerformTest, self).setUp()
 
-        with mock.patch("letsencrypt_apache.configurator.ApacheConfigurator."
-                        "mod_loaded") as mock_load:
-            mock_load.return_value = True
-            config = util.get_apache_configurator(
-                self.config_path, self.config_dir, self.work_dir)
+        config = util.get_apache_configurator(
+            self.config_path, self.config_dir, self.work_dir)
+        config.config.dvsni_port = 443
 
         from letsencrypt_apache import dvsni
         self.sni = dvsni.ApacheDvsni(config)
@@ -38,7 +36,11 @@ class DvsniPerformTest(util.ApacheTest):
         resp = self.sni.perform()
         self.assertEqual(len(resp), 0)
 
-    def test_perform1(self):
+    @mock.patch("letsencrypt_apache.parser.subprocess.Popen")
+    def test_perform1(self, mock_popen):
+        mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "")
+        mock_popen().returncode = 0
+
         achall = self.achalls[0]
         self.sni.add_chall(achall)
         mock_setup_cert = mock.MagicMock(
@@ -53,12 +55,14 @@ class DvsniPerformTest(util.ApacheTest):
         # Check to make sure challenge config path is included in apache config.
         self.assertEqual(
             len(self.sni.configurator.parser.find_dir(
-                "Include", self.sni.challenge_conf)),
-            1)
+                "Include", self.sni.challenge_conf)), 1)
         self.assertEqual(len(responses), 1)
         self.assertEqual(responses[0].s, "randomS1")
 
     def test_perform2(self):
+        # Avoid load module
+        self.sni.configurator.parser.modules.add("ssl_module")
+
         for achall in self.achalls:
             self.sni.add_chall(achall)
 
@@ -89,13 +93,8 @@ class DvsniPerformTest(util.ApacheTest):
     def test_mod_config(self):
         for achall in self.achalls:
             self.sni.add_chall(achall)
-        v_addr1 = [common.Addr(("1.2.3.4", "443")),
-                   common.Addr(("5.6.7.8", "443"))]
-        v_addr2 = [common.Addr(("127.0.0.1", "443"))]
-        ll_addr = []
-        ll_addr.append(v_addr1)
-        ll_addr.append(v_addr2)
-        self.sni._mod_config(ll_addr)  # pylint: disable=protected-access
+
+        self.sni._mod_config()  # pylint: disable=protected-access
         self.sni.configurator.save()
 
         self.sni.configurator.parser.find_dir(
@@ -109,15 +108,10 @@ class DvsniPerformTest(util.ApacheTest):
             vhs.append(self.sni.configurator._create_vhost(match))
         self.assertEqual(len(vhs), 2)
         for vhost in vhs:
-            if vhost.addrs == set(v_addr1):
-                self.assertEqual(
-                    vhost.names,
-                    set([self.achalls[0].nonce_domain]))
-            else:
-                self.assertEqual(vhost.addrs, set(v_addr2))
-                self.assertEqual(
-                    vhost.names,
-                    set([self.achalls[1].nonce_domain]))
+            self.assertEqual(vhost.addrs, set([obj.Addr.fromstring("*:443")]))
+            self.assertTrue(
+                vhost.names == set([self.achalls[0].nonce_domain]) or
+                vhost.names == set([self.achalls[1].nonce_domain]))
 
 
 if __name__ == "__main__":

letsencrypt-apache/letsencrypt_apache/tests/parser_test.py

@@ -54,11 +54,11 @@ class ApacheParserTest(util.ApacheTest):
         self.assertTrue(matches)
 
     def test_find_dir(self):
-        from letsencrypt_apache.parser import case_i
-        test = self.parser.find_dir(case_i("Listen"), "443")
+        test = self.parser.find_dir("Listen", "80")
         # This will only look in enabled hosts
-        test2 = self.parser.find_dir(case_i("documentroot"))
-        self.assertEqual(len(test), 2)
+        test2 = self.parser.find_dir("documentroot")
+
+        self.assertEqual(len(test), 1)
         self.assertEqual(len(test2), 3)
 
     def test_add_dir(self):
@@ -80,6 +80,9 @@ class ApacheParserTest(util.ApacheTest):
 
         """
         from letsencrypt_apache.parser import get_aug_path
+        # This makes sure that find_dir will work
+        self.parser.modules.add("mod_ssl.c")
+
         self.parser.add_dir_to_ifmodssl(
             get_aug_path(self.parser.loc["default"]),
             "FakeDirective", ["123"])

letsencrypt-apache/letsencrypt_apache/tests/util.py

@@ -35,8 +35,7 @@ class ApacheTest(unittest.TestCase):  # pylint: disable=too-few-public-methods
 
 
 def get_apache_configurator(
-        config_path, config_dir, work_dir, version=(2, 4, 7),
-        conf=mock.MagicMock()):
+        config_path, config_dir, work_dir, version=(2, 4, 7), conf=None):
     """Create an Apache Configurator with the specified options.
 
     :param conf: Function that returns binary paths. self.conf in Configurator
@@ -44,25 +43,30 @@ def get_apache_configurator(
     """
     backups = os.path.join(work_dir, "backups")
 
-    with mock.patch("letsencrypt_apache.configurator.subprocess.Popen") as mock_popen:
-        # This indicates config_test passes
-        mock_popen().communicate.return_value = ("Fine output", "No problems")
-        mock_popen.returncode.return_value = 0
-        # mock_popen().communicate.return_value = ("ssl_module", "")
-        config = configurator.ApacheConfigurator(
-            config=mock.MagicMock(
-                apache_server_root=config_path,
-                apache_le_vhost_ext=constants.CLI_DEFAULTS["le_vhost_ext"],
-                backup_dir=backups,
-                config_dir=config_dir,
-                temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"),
-                in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
-                work_dir=work_dir),
-            name="apache",
-            version=version)
-        config.conf = conf
+    with mock.patch("letsencrypt_apache.configurator."
+                    "subprocess.Popen") as mock_popen:
+        with mock.patch("letsencrypt_apache.parser.ApacheParser."
+                        "update_runtime_variables"):
+            # This indicates config_test passes
+            mock_popen().communicate.return_value = ("Fine output", "No problems")
+            mock_popen().returncode = 0
 
-    config.prepare()
+            config = configurator.ApacheConfigurator(
+                config=mock.MagicMock(
+                    apache_server_root=config_path,
+                    apache_le_vhost_ext=constants.CLI_DEFAULTS["le_vhost_ext"],
+                    backup_dir=backups,
+                    config_dir=config_dir,
+                    temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"),
+                    in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
+                    work_dir=work_dir),
+                name="apache",
+                version=version)
+            # This allows testing scripts to set it a bit more quickly
+            if conf is not None:
+                config.conf = conf
+
+            config.prepare()
 
     return config
 
@@ -77,21 +81,21 @@ def get_vh_truth(temp_dir, config_name):
             obj.VirtualHost(
                 os.path.join(prefix, "encryption-example.conf"),
                 os.path.join(aug_pre, "encryption-example.conf/VirtualHost"),
-                set([common.Addr.fromstring("*:80")]),
+                set([obj.Addr.fromstring("*:80")]),
                 False, True, set(["encryption-example.demo"])),
             obj.VirtualHost(
                 os.path.join(prefix, "default-ssl.conf"),
                 os.path.join(aug_pre, "default-ssl.conf/IfModule/VirtualHost"),
-                set([common.Addr.fromstring("_default_:443")]), True, False),
+                set([obj.Addr.fromstring("_default_:443")]), True, False),
             obj.VirtualHost(
                 os.path.join(prefix, "000-default.conf"),
                 os.path.join(aug_pre, "000-default.conf/VirtualHost"),
-                set([common.Addr.fromstring("*:80")]), False, True,
+                set([obj.Addr.fromstring("*:80")]), False, True,
                 set(["ip-172-30-0-17"])),
             obj.VirtualHost(
                 os.path.join(prefix, "letsencrypt.conf"),
                 os.path.join(aug_pre, "letsencrypt.conf/VirtualHost"),
-                set([common.Addr.fromstring("*:80")]), False, True,
+                set([obj.Addr.fromstring("*:80")]), False, True,
                 set(["letsencrypt.demo"])),
         ]
         return vh_truth