Merge pull request #4607 from certbot/joohoi_multiple_vhosts2

Multiple vhosts 2

certbot-apache/certbot_apache/augeas_configurator.py

@@ -47,7 +47,9 @@ class AugeasConfigurator(common.Plugin):
             loadpath=constants.AUGEAS_LENS_DIR,
             # Do not save backup (we do it ourselves), do not load
             # anything by default
-            flags=(augeas.Augeas.NONE | augeas.Augeas.NO_MODL_AUTOLOAD))
+            flags=(augeas.Augeas.NONE |
+                   augeas.Augeas.NO_MODL_AUTOLOAD |
+                   augeas.Augeas.ENABLE_SPAN))
         self.recovery_routine()
 
     def check_parsing_errors(self, lens):

certbot-apache/certbot_apache/configurator.py

@@ -502,6 +502,32 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         return ""
 
+    def _get_vhost_names(self, path):
+        """Helper method for getting the ServerName and
+        ServerAlias values from vhost in path
+
+        :param path: Path to read ServerName and ServerAliases from
+
+        :returns: Tuple including ServerName and `list` of ServerAlias strings
+        """
+
+        servername_match = self.parser.find_dir(
+            "ServerName", None, start=path, exclude=False)
+        serveralias_match = self.parser.find_dir(
+            "ServerAlias", None, start=path, exclude=False)
+
+        serveraliases = []
+        for alias in serveralias_match:
+            serveralias = self.parser.get_arg(alias)
+            serveraliases.append(serveralias)
+
+        servername = None
+        if servername_match:
+            # Get last ServerName as each overwrites the previous
+            servername = self.parser.get_arg(servername_match[-1])
+
+        return (servername, serveraliases)
+
     def _add_servernames(self, host):
         """Helper function for get_virtual_hosts().
 
@@ -509,22 +535,15 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         :type host: :class:`~certbot_apache.obj.VirtualHost`
 
         """
-        # Take the final ServerName as each overrides the previous
-        servername_match = self.parser.find_dir(
-            "ServerName", None, start=host.path, exclude=False)
-        serveralias_match = self.parser.find_dir(
-            "ServerAlias", None, start=host.path, exclude=False)
 
-        for alias in serveralias_match:
-            serveralias = self.parser.get_arg(alias)
-            if not host.modmacro:
-                host.aliases.add(serveralias)
+        servername, serveraliases = self._get_vhost_names(host.path)
 
-        if servername_match:
-            # Get last ServerName as each overwrites the previous
-            servername = self.parser.get_arg(servername_match[-1])
+        for alias in serveraliases:
             if not host.modmacro:
-                host.name = servername
+                host.aliases.add(alias)
+
+        if not host.modmacro:
+            host.name = servername
 
     def _create_vhost(self, path):
         """Used by get_virtual_hosts to create vhost objects
@@ -581,30 +600,46 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         """
         # Search base config, and all included paths for VirtualHosts
+        file_paths = {}
+        internal_paths = defaultdict(set)
         vhs = []
-        vhost_paths = {}
         for vhost_path in self.parser.parser_paths.keys():
             paths = self.aug.match(
                 ("/files%s//*[label()=~regexp('%s')]" %
                     (vhost_path, parser.case_i("VirtualHost"))))
             paths = [path for path in paths if
-                     os.path.basename(path.lower()) == "virtualhost"]
+                     "virtualhost" in os.path.basename(path).lower()]
             for path in paths:
                 new_vhost = self._create_vhost(path)
                 if not new_vhost:
                     continue
+                internal_path = get_internal_aug_path(new_vhost.path)
                 realpath = os.path.realpath(new_vhost.filep)
-                if realpath not in vhost_paths.keys():
+                if realpath not in file_paths:
+                    file_paths[realpath] = new_vhost.filep
+                    internal_paths[realpath].add(internal_path)
                     vhs.append(new_vhost)
-                    vhost_paths[realpath] = new_vhost.filep
-                elif realpath == new_vhost.filep:
+                elif (realpath == new_vhost.filep and
+                      realpath != file_paths[realpath]):
                     # Prefer "real" vhost paths instead of symlinked ones
                     # ex: sites-enabled/vh.conf -> sites-available/vh.conf
 
                     # remove old (most likely) symlinked one
-                    vhs = [v for v in vhs if v.filep != vhost_paths[realpath]]
+                    new_vhs = []
+                    for v in vhs:
+                        if v.filep == file_paths[realpath]:
+                            internal_paths[realpath].remove(
+                                get_internal_aug_path(v.path))
+                        else:
+                            new_vhs.append(v)
+                    vhs = new_vhs
+
+                    file_paths[realpath] = realpath
+                    internal_paths[realpath].add(internal_path)
+                    vhs.append(new_vhost)
+                elif internal_path not in internal_paths[realpath]:
+                    internal_paths[realpath].add(internal_path)
                     vhs.append(new_vhost)
-                    vhost_paths[realpath] = realpath
 
         return vhs
 
@@ -800,24 +835,27 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         avail_fp = nonssl_vhost.filep
         ssl_fp = self._get_ssl_vhost_path(avail_fp)
 
-        self._copy_create_ssl_vhost_skeleton(avail_fp, ssl_fp)
+        orig_matches = self.aug.match("/files%s//* [label()=~regexp('%s')]" %
+                                      (self._escape(ssl_fp),
+                                       parser.case_i("VirtualHost")))
+
+        self._copy_create_ssl_vhost_skeleton(nonssl_vhost, ssl_fp)
 
         # Reload augeas to take into account the new vhost
         self.aug.load()
         # Get Vhost augeas path for new vhost
-        vh_p = self.aug.match("/files%s//* [label()=~regexp('%s')]" %
-                              (self._escape(ssl_fp), parser.case_i("VirtualHost")))
-        if len(vh_p) != 1:
-            logger.error("Error: should only be one vhost in %s", avail_fp)
-            raise errors.PluginError("Currently, we only support "
-                                     "configurations with one vhost per file")
-        else:
-            # This simplifies the process
-            vh_p = vh_p[0]
+        new_matches = self.aug.match("/files%s//* [label()=~regexp('%s')]" %
+                                     (self._escape(ssl_fp),
+                                      parser.case_i("VirtualHost")))
+
+        vh_p = self._get_new_vh_path(orig_matches, new_matches)
+
+        if not vh_p:
+            raise errors.PluginError(
+                "Could not reverse map the HTTPS VirtualHost to the original")
 
         # Update Addresses
         self._update_ssl_vhosts_addrs(vh_p)
-
         # Add directives
         self._add_dummy_ssl_directives(vh_p)
         self.save()
@@ -830,6 +868,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)
+        ssl_vhost.ancestor = nonssl_vhost
         self.vhosts.append(ssl_vhost)
 
         # NOTE: Searches through Augeas seem to ruin changes to directives
@@ -843,6 +882,20 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         return ssl_vhost
 
+    def _get_new_vh_path(self, orig_matches, new_matches):
+        """ Helper method for make_vhost_ssl for matching augeas paths. Returns
+        VirtualHost path from new_matches that's not present in orig_matches.
+
+        Paths are normalized, because augeas leaves indices out for paths
+        with only single directive with a similar key """
+
+        orig_matches = [i.replace("[1]", "") for i in orig_matches]
+        for match in new_matches:
+            if match.replace("[1]", "") not in orig_matches:
+                # Return the unmodified path
+                return match
+        return None
+
     def _get_ssl_vhost_path(self, non_ssl_vh_fp):
         # Get filepath of new ssl_vhost
         if non_ssl_vh_fp.endswith(".conf"):
@@ -867,7 +920,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         :rtype: bool
 
         """
-        if not line.lstrip().startswith("RewriteRule"):
+        if not line.lower().lstrip().startswith("rewriterule"):
             return False
 
         # According to: http://httpd.apache.org/docs/2.4/rewrite/flags.html
@@ -883,10 +936,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # Sift line if it redirects the request to a HTTPS site
         return target.startswith("https://")
 
-    def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp):
+    def _copy_create_ssl_vhost_skeleton(self, vhost, ssl_fp):
         """Copies over existing Vhost with IfModule mod_ssl.c> skeleton.
 
-        :param str avail_fp: Pointer to the original available non-ssl vhost
+        :param obj.VirtualHost vhost: Original VirtualHost object
         :param str ssl_fp: Full path where the new ssl_vhost will reside.
 
         A new file is created on the filesystem.
@@ -894,70 +947,25 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         """
         # First register the creation so that it is properly removed if
         # configuration is rolled back
-        self.reverter.register_file_creation(False, ssl_fp)
+        if os.path.exists(ssl_fp):
+            notes = "Appended new VirtualHost directive to file %s" % ssl_fp
+            files = set()
+            files.add(ssl_fp)
+            self.reverter.add_to_checkpoint(files, notes)
+        else:
+            self.reverter.register_file_creation(False, ssl_fp)
         sift = False
 
         try:
-            with open(avail_fp, "r") as orig_file:
-                with open(ssl_fp, "w") as new_file:
-                    new_file.write("<IfModule mod_ssl.c>\n")
+            orig_contents = self._get_vhost_block(vhost)
+            ssl_vh_contents, sift = self._sift_rewrite_rules(orig_contents)
 
-                    comment = ("# Some rewrite rules in this file were "
-                              "disabled on your HTTPS site,\n"
-                              "# because they have the potential to create "
-                              "redirection loops.\n")
-
-                    for line in orig_file:
-                        A = line.lstrip().startswith("RewriteCond")
-                        B = line.lstrip().startswith("RewriteRule")
-
-                        if not (A or B):
-                            new_file.write(line)
-                            continue
-
-                        # A RewriteRule that doesn't need filtering
-                        if B and not self._sift_rewrite_rule(line):
-                            new_file.write(line)
-                            continue
-
-                        # A RewriteRule that does need filtering
-                        if B and self._sift_rewrite_rule(line):
-                            if not sift:
-                                new_file.write(comment)
-                                sift = True
-                            new_file.write("# " + line)
-                            continue
-
-                        # We save RewriteCond(s) and their corresponding
-                        # RewriteRule in 'chunk'.
-                        # We then decide whether we comment out the entire
-                        # chunk based on its RewriteRule.
-                        chunk = []
-                        if A:
-                            chunk.append(line)
-                            line = next(orig_file)
-
-                            # RewriteCond(s) must be followed by one RewriteRule
-                            while not line.lstrip().startswith("RewriteRule"):
-                                chunk.append(line)
-                                line = next(orig_file)
-
-                            # Now, current line must start with a RewriteRule
-                            chunk.append(line)
-
-                            if self._sift_rewrite_rule(line):
-                                if not sift:
-                                    new_file.write(comment)
-                                    sift = True
-
-                                new_file.write(''.join(
-                                    ['# ' + l for l in chunk]))
-                                continue
-                            else:
-                                new_file.write(''.join(chunk))
-                                continue
-
-                    new_file.write("</IfModule>\n")
+            with open(ssl_fp, "a") as new_file:
+                new_file.write("<IfModule mod_ssl.c>\n")
+                new_file.write("\n".join(ssl_vh_contents))
+                # The content does not include the closing tag, so add it
+                new_file.write("</VirtualHost>\n")
+                new_file.write("</IfModule>\n")
         except IOError:
             logger.fatal("Error writing/reading to file in make_vhost_ssl")
             raise errors.PluginError("Unable to write/read in make_vhost_ssl")
@@ -967,9 +975,96 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             reporter.add_message(
                 "Some rewrite rules copied from {0} were disabled in the "
                 "vhost for your HTTPS site located at {1} because they have "
-                "the potential to create redirection loops.".format(avail_fp,
-                                                                    ssl_fp),
-                reporter.MEDIUM_PRIORITY)
+                "the potential to create redirection loops.".format(
+                    vhost.filep, ssl_fp), reporter.MEDIUM_PRIORITY)
+        self.aug.set("/augeas/files%s/mtime" % (self._escape(ssl_fp)), "0")
+        self.aug.set("/augeas/files%s/mtime" % (self._escape(vhost.filep)), "0")
+
+    def _sift_rewrite_rules(self, contents):
+        """ Helper function for _copy_create_ssl_vhost_skeleton to prepare the
+        new HTTPS VirtualHost contents. Currently disabling the rewrites """
+
+        result = []
+        sift = False
+        contents = iter(contents)
+
+        comment = ("# Some rewrite rules in this file were "
+                   "disabled on your HTTPS site,\n"
+                   "# because they have the potential to create "
+                   "redirection loops.\n")
+
+        for line in contents:
+            A = line.lower().lstrip().startswith("rewritecond")
+            B = line.lower().lstrip().startswith("rewriterule")
+
+            if not (A or B):
+                result.append(line)
+                continue
+
+            # A RewriteRule that doesn't need filtering
+            if B and not self._sift_rewrite_rule(line):
+                result.append(line)
+                continue
+
+            # A RewriteRule that does need filtering
+            if B and self._sift_rewrite_rule(line):
+                if not sift:
+                    result.append(comment)
+                    sift = True
+                result.append("# " + line)
+                continue
+
+            # We save RewriteCond(s) and their corresponding
+            # RewriteRule in 'chunk'.
+            # We then decide whether we comment out the entire
+            # chunk based on its RewriteRule.
+            chunk = []
+            if A:
+                chunk.append(line)
+                line = next(contents)
+
+                # RewriteCond(s) must be followed by one RewriteRule
+                while not line.lower().lstrip().startswith("rewriterule"):
+                    chunk.append(line)
+                    line = next(contents)
+
+                # Now, current line must start with a RewriteRule
+                chunk.append(line)
+
+                if self._sift_rewrite_rule(line):
+                    if not sift:
+                        result.append(comment)
+                        sift = True
+
+                    result.append('\n'.join(
+                        ['# ' + l for l in chunk]))
+                    continue
+                else:
+                    result.append('\n'.join(chunk))
+                    continue
+        return result, sift
+
+    def _get_vhost_block(self, vhost):
+        """ Helper method to get VirtualHost contents from the original file.
+        This is done with help of augeas span, which returns the span start and
+        end positions
+
+        :returns: `list` of VirtualHost block content lines without closing tag
+        """
+
+        try:
+            span_val = self.aug.span(vhost.path)
+        except ValueError:
+            logger.fatal("Error while reading the VirtualHost %s from "
+                         "file %s", vhost.name, vhost.filep, exc_info=True)
+            raise errors.PluginError("Unable to read VirtualHost from file")
+        span_filep = span_val[0]
+        span_start = span_val[5]
+        span_end = span_val[6]
+        with open(span_filep, 'r') as fh:
+            fh.seek(span_start)
+            vh_contents = fh.read(span_end-span_start)
+        return vh_contents.split("\n")
 
     def _update_ssl_vhosts_addrs(self, vh_path):
         ssl_addrs = set()
@@ -1016,16 +1111,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         self.parser.add_dir(vh_path, "Include", self.mod_ssl_conf)
 
     def _add_servername_alias(self, target_name, vhost):
-        fp = self._escape(vhost.filep)
-        vh_p = self.aug.match("/files%s//* [label()=~regexp('%s')]" %
-                              (fp, parser.case_i("VirtualHost")))
-        if not vh_p:
-            return
-        vh_path = vh_p[0]
-        if (self.parser.find_dir("ServerName", target_name,
-                                 start=vh_path, exclude=False) or
-            self.parser.find_dir("ServerAlias", target_name,
-                                 start=vh_path, exclude=False)):
+        vh_path = vhost.path
+        sname, saliases = self._get_vhost_names(vh_path)
+        if target_name == sname or target_name in saliases:
             return
         if self._has_matching_wildcard(vh_path, target_name):
             return
@@ -1425,7 +1513,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             for re_path in rewrite_engine_path_list:
                 # A RewriteEngine directive may also be included in per
                 # directory .htaccess files. We only care about the VirtualHost.
-                if 'VirtualHost' in re_path:
+                if 'virtualhost' in re_path.lower():
                     return self.parser.get_arg(re_path)
         return False
 
@@ -1516,6 +1604,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
     def _get_http_vhost(self, ssl_vhost):
         """Find appropriate HTTP vhost for ssl_vhost."""
         # First candidate vhosts filter
+        if ssl_vhost.ancestor:
+            return ssl_vhost.ancestor
         candidate_http_vhs = [
             vhost for vhost in self.vhosts if not vhost.ssl
         ]
@@ -1827,25 +1917,46 @@ def get_file_path(vhost_path):
     :rtype: str
 
     """
-    # Strip off /files/
-    try:
-        if vhost_path.startswith("/files/"):
-            avail_fp = vhost_path[7:].split("/")
-        else:
-            return None
-    except AttributeError:
-        # If we received a None path
+    if not vhost_path or not vhost_path.startswith("/files/"):
         return None
 
-    last_good = ""
-    # Loop through the path parts and validate after every addition
-    for p in avail_fp:
-        cur_path = last_good+"/"+p
-        if os.path.exists(cur_path):
-            last_good = cur_path
-        else:
-            break
-    return last_good
+    return _split_aug_path(vhost_path)[0]
+
+
+def get_internal_aug_path(vhost_path):
+    """Get the Augeas path for a vhost with the file path removed.
+
+    :param str vhost_path: Augeas virtual host path
+
+    :returns: Augeas path to vhost relative to the containing file
+    :rtype: str
+
+    """
+    return _split_aug_path(vhost_path)[1]
+
+
+def _split_aug_path(vhost_path):
+    """Splits an Augeas path into a file path and an internal path.
+
+    After removing "/files", this function splits vhost_path into the
+    file path and the remaining Augeas path.
+
+    :param str vhost_path: Augeas virtual host path
+
+    :returns: file path and internal Augeas path
+    :rtype: `tuple` of `str`
+
+    """
+    # Strip off /files
+    file_path = vhost_path[6:]
+    internal_path = []
+
+    # Remove components from the end of file_path until it becomes valid
+    while not os.path.exists(file_path):
+        file_path, _, internal_path_part = file_path.rpartition("/")
+        internal_path.append(internal_path_part)
+
+    return file_path, "/".join(reversed(internal_path))
 
 
 def install_ssl_options_conf(options_ssl):

certbot-apache/certbot_apache/obj.py

@@ -112,6 +112,7 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
     :ivar bool ssl: SSLEngine on in vhost
     :ivar bool enabled: Virtual host is enabled
     :ivar bool modmacro: VirtualHost is using mod_macro
+    :ivar VirtualHost ancestor: A non-SSL VirtualHost this is based on
 
     https://httpd.apache.org/docs/2.4/vhosts/details.html
 
@@ -123,7 +124,7 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
     strip_name = re.compile(r"^(?:.+://)?([^ :$]*)")
 
     def __init__(self, filep, path, addrs, ssl, enabled, name=None,
-                 aliases=None, modmacro=False):
+                 aliases=None, modmacro=False, ancestor=None):
 
         # pylint: disable=too-many-arguments
         """Initialize a VH."""
@@ -135,6 +136,7 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
         self.ssl = ssl
         self.enabled = enabled
         self.modmacro = modmacro
+        self.ancestor = ancestor
 
     def get_names(self):
         """Return a set of all names."""

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -155,6 +155,17 @@ class MultipleVhostsTest(util.ApacheTest):
         self.assertEqual(get_file_path("nonexistent"), None)
         self.assertEqual(self.config._create_vhost("nonexistent"), None) # pylint: disable=protected-access
 
+    def test_get_aug_internal_path(self):
+        from certbot_apache.configurator import get_internal_aug_path
+        internal_paths = [
+            "VirtualHost", "IfModule/VirtualHost", "VirtualHost", "VirtualHost",
+            "Macro/VirtualHost", "IfModule/VirtualHost", "VirtualHost",
+            "IfModule/VirtualHost"]
+
+        for i, internal_path in enumerate(internal_paths):
+            self.assertEqual(
+                get_internal_aug_path(self.vh_truth[i].path), internal_path)
+
     def test_bad_servername_alias(self):
         ssl_vh1 = obj.VirtualHost(
             "fp1", "ap1", set([obj.Addr(("*", "443"))]),
@@ -586,6 +597,7 @@ class MultipleVhostsTest(util.ApacheTest):
         # already listens to the correct port
         self.assertEqual(mock_add_dir.call_count, 0)
 
+
     def test_make_vhost_ssl(self):
         ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0])
 
@@ -672,11 +684,6 @@ class MultipleVhostsTest(util.ApacheTest):
                 len(self.config.parser.find_dir(
                     directive, None, self.vh_truth[1].path, False)), 0)
 
-    def test_make_vhost_ssl_extra_vhs(self):
-        self.config.aug.match = mock.Mock(return_value=["p1", "p2"])
-        self.assertRaises(
-            errors.PluginError, self.config.make_vhost_ssl, self.vh_truth[0])
-
     def test_make_vhost_ssl_bad_write(self):
         mock_open = mock.mock_open()
         # This calls open
@@ -811,6 +818,15 @@ class MultipleVhostsTest(util.ApacheTest):
     def test_supported_enhancements(self):
         self.assertTrue(isinstance(self.config.supported_enhancements(), list))
 
+    def test_find_http_vhost_without_ancestor(self):
+        # pylint: disable=protected-access
+        vhost = self.vh_truth[0]
+        vhost.ssl = True
+        vhost.ancestor = None
+        res = self.config._get_http_vhost(vhost)
+        self.assertEqual(self.vh_truth[0].name, res.name)
+        self.assertEqual(self.vh_truth[0].aliases, res.aliases)
+
     @mock.patch("certbot_apache.configurator.ApacheConfigurator._get_http_vhost")
     @mock.patch("certbot_apache.display_ops.select_vhost")
     @mock.patch("certbot.util.exe_exists")
@@ -1010,8 +1026,9 @@ class MultipleVhostsTest(util.ApacheTest):
         # three args to rw_rule
         self.assertEqual(len(rw_rule), 3)
 
-        self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path))
-        self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path))
+        # [:-3] to remove the vhost index number
+        self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path[:-3]))
+        self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path[:-3]))
 
         self.assertTrue("rewrite_module" in self.config.parser.modules)
 
@@ -1059,9 +1076,9 @@ class MultipleVhostsTest(util.ApacheTest):
         self.assertEqual(len(rw_engine), 1)
         # three args to rw_rule + 1 arg for the pre existing rewrite
         self.assertEqual(len(rw_rule), 5)
-
-        self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path))
-        self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path))
+        # [:-3] to remove the vhost index number
+        self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path[:-3]))
+        self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path[:-3]))
 
         self.assertTrue("rewrite_module" in self.config.parser.modules)
 
@@ -1169,89 +1186,6 @@ class MultipleVhostsTest(util.ApacheTest):
         not_rewriterule = "NotRewriteRule ^ ..."
         self.assertFalse(self.config._sift_rewrite_rule(not_rewriterule))
 
-    @certbot_util.patch_get_utility()
-    def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility):
-        self.config.parser.modules.add("rewrite_module")
-
-        http_vhost = self.vh_truth[0]
-
-        self.config.parser.add_dir(
-            http_vhost.path, "RewriteEngine", "on")
-
-        self.config.parser.add_dir(
-            http_vhost.path, "RewriteRule",
-            ["^",
-             "https://%{SERVER_NAME}%{REQUEST_URI}",
-             "[L,NE,R=permanent]"])
-        self.config.save()
-
-        ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0])
-
-        self.assertTrue(self.config.parser.find_dir(
-            "RewriteEngine", "on", ssl_vhost.path, False))
-
-        conf_text = open(ssl_vhost.filep).read()
-        commented_rewrite_rule = ("# RewriteRule ^ "
-                                  "https://%{SERVER_NAME}%{REQUEST_URI} "
-                                  "[L,NE,R=permanent]")
-        self.assertTrue(commented_rewrite_rule in conf_text)
-        mock_get_utility().add_message.assert_called_once_with(mock.ANY,
-
-                                                               mock.ANY)
-    @certbot_util.patch_get_utility()
-    def test_make_vhost_ssl_with_existing_rewrite_conds(self, mock_get_utility):
-        self.config.parser.modules.add("rewrite_module")
-
-        http_vhost = self.vh_truth[0]
-
-        self.config.parser.add_dir(
-            http_vhost.path, "RewriteEngine", "on")
-
-        # Add a chunk that should not be commented out.
-        self.config.parser.add_dir(http_vhost.path,
-                "RewriteCond", ["%{DOCUMENT_ROOT}/%{REQUEST_FILENAME}", "!-f"])
-        self.config.parser.add_dir(
-            http_vhost.path, "RewriteRule",
-            ["^(.*)$", "b://u%{REQUEST_URI}", "[P,NE,L]"])
-
-        # Add a chunk that should be commented out.
-        self.config.parser.add_dir(http_vhost.path,
-                "RewriteCond", ["%{HTTPS}", "!=on"])
-        self.config.parser.add_dir(http_vhost.path,
-                "RewriteCond", ["%{HTTPS}", "!^$"])
-        self.config.parser.add_dir(
-            http_vhost.path, "RewriteRule",
-            ["^",
-             "https://%{SERVER_NAME}%{REQUEST_URI}",
-             "[L,NE,R=permanent]"])
-
-        self.config.save()
-
-        ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0])
-
-        conf_line_set = set(open(ssl_vhost.filep).read().splitlines())
-
-        not_commented_cond1 = ("RewriteCond "
-                "%{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f")
-        not_commented_rewrite_rule = ("RewriteRule "
-            "^(.*)$ b://u%{REQUEST_URI} [P,NE,L]")
-
-        commented_cond1 = "# RewriteCond %{HTTPS} !=on"
-        commented_cond2 = "# RewriteCond %{HTTPS} !^$"
-        commented_rewrite_rule = ("# RewriteRule ^ "
-                                  "https://%{SERVER_NAME}%{REQUEST_URI} "
-                                  "[L,NE,R=permanent]")
-
-        self.assertTrue(not_commented_cond1 in conf_line_set)
-        self.assertTrue(not_commented_rewrite_rule in conf_line_set)
-
-        self.assertTrue(commented_cond1 in conf_line_set)
-        self.assertTrue(commented_cond2 in conf_line_set)
-        self.assertTrue(commented_rewrite_rule in conf_line_set)
-        mock_get_utility().add_message.assert_called_once_with(mock.ANY,
-                                                               mock.ANY)
-
-
     def get_achalls(self):
         """Return testing achallenges."""
         account_key = self.rsa512jwk
@@ -1362,6 +1296,126 @@ class AugeasVhostsTest(util.ApacheTest):
                 self.config.choose_vhost(name)
                 self.assertEqual(mock_select.call_count, 0)
 
+    def test_augeas_span_error(self):
+        broken_vhost = self.config.vhosts[0]
+        broken_vhost.path = broken_vhost.path + "/nonexistent"
+        self.assertRaises(errors.PluginError, self.config.make_vhost_ssl,
+                          broken_vhost)
+
+class MultiVhostsTest(util.ApacheTest):
+    """Test vhosts with illegal names dependant on augeas version."""
+    # pylint: disable=protected-access
+
+    def setUp(self):  # pylint: disable=arguments-differ
+        td = "debian_apache_2_4/multi_vhosts"
+        cr = "debian_apache_2_4/multi_vhosts/apache2"
+        vr = "debian_apache_2_4/multi_vhosts/apache2/sites-available"
+        super(MultiVhostsTest, self).setUp(test_dir=td,
+                                            config_root=cr,
+                                            vhost_root=vr)
+
+        self.config = util.get_apache_configurator(
+            self.config_path, self.vhost_path, self.config_dir, self.work_dir)
+        self.vh_truth = util.get_vh_truth(
+            self.temp_dir, "debian_apache_2_4/multi_vhosts")
+
+    def tearDown(self):
+        shutil.rmtree(self.temp_dir)
+        shutil.rmtree(self.config_dir)
+        shutil.rmtree(self.work_dir)
+
+    def test_make_vhost_ssl(self):
+        ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[1])
+
+        self.assertEqual(
+            ssl_vhost.filep,
+            os.path.join(self.config_path, "sites-available",
+                         "default-le-ssl.conf"))
+
+        self.assertEqual(ssl_vhost.path,
+                         "/files" + ssl_vhost.filep + "/IfModule/VirtualHost")
+        self.assertEqual(len(ssl_vhost.addrs), 1)
+        self.assertEqual(set([obj.Addr.fromstring("*:443")]), ssl_vhost.addrs)
+        self.assertEqual(ssl_vhost.name, "banana.vomit.com")
+        self.assertTrue(ssl_vhost.ssl)
+        self.assertFalse(ssl_vhost.enabled)
+
+        self.assertTrue(self.config.parser.find_dir(
+            "SSLCertificateFile", None, ssl_vhost.path, False))
+        self.assertTrue(self.config.parser.find_dir(
+            "SSLCertificateKeyFile", None, ssl_vhost.path, False))
+
+        self.assertEqual(self.config.is_name_vhost(self.vh_truth[1]),
+                         self.config.is_name_vhost(ssl_vhost))
+
+        mock_path = "certbot_apache.configurator.ApacheConfigurator._get_new_vh_path"
+        with mock.patch(mock_path) as mock_getpath:
+            mock_getpath.return_value = None
+            self.assertRaises(errors.PluginError, self.config.make_vhost_ssl,
+                              self.vh_truth[1])
+
+    def test_get_new_path(self):
+        with_index_1 = ["/path[1]/section[1]"]
+        without_index = ["/path/section"]
+        with_index_2 = ["/path[2]/section[2]"]
+        self.assertEqual(self.config._get_new_vh_path(without_index,
+                                                      with_index_1),
+                         None)
+        self.assertEqual(self.config._get_new_vh_path(without_index,
+                                                      with_index_2),
+                         with_index_2[0])
+
+        both = with_index_1 + with_index_2
+        self.assertEqual(self.config._get_new_vh_path(without_index, both),
+                         with_index_2[0])
+
+    @certbot_util.patch_get_utility()
+    def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility):
+        self.config.parser.modules.add("rewrite_module")
+
+        ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[4])
+
+        self.assertTrue(self.config.parser.find_dir(
+            "RewriteEngine", "on", ssl_vhost.path, False))
+
+        conf_text = open(ssl_vhost.filep).read()
+        commented_rewrite_rule = ("# RewriteRule \"^/secrets/(.+)\" "
+                                  "\"https://new.example.com/docs/$1\" [R,L]")
+        uncommented_rewrite_rule = ("RewriteRule \"^/docs/(.+)\"  "
+                                    "\"http://new.example.com/docs/$1\"  [R,L]")
+        self.assertTrue(commented_rewrite_rule in conf_text)
+        self.assertTrue(uncommented_rewrite_rule in conf_text)
+        mock_get_utility().add_message.assert_called_once_with(mock.ANY,
+                                                               mock.ANY)
+
+    @certbot_util.patch_get_utility()
+    def test_make_vhost_ssl_with_existing_rewrite_conds(self, mock_get_utility):
+        self.config.parser.modules.add("rewrite_module")
+
+        ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[3])
+
+        conf_lines = open(ssl_vhost.filep).readlines()
+        conf_line_set = [l.strip() for l in conf_lines]
+        not_commented_cond1 = ("RewriteCond "
+                "%{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f")
+        not_commented_rewrite_rule = ("RewriteRule "
+            "^(.*)$ b://u%{REQUEST_URI} [P,NE,L]")
+
+        commented_cond1 = "# RewriteCond %{HTTPS} !=on"
+        commented_cond2 = "# RewriteCond %{HTTPS} !^$"
+        commented_rewrite_rule = ("# RewriteRule ^ "
+                                  "https://%{SERVER_NAME}%{REQUEST_URI} "
+                                  "[L,NE,R=permanent]")
+
+        self.assertTrue(not_commented_cond1 in conf_line_set)
+        self.assertTrue(not_commented_rewrite_rule in conf_line_set)
+
+        self.assertTrue(commented_cond1 in conf_line_set)
+        self.assertTrue(commented_cond2 in conf_line_set)
+        self.assertTrue(commented_rewrite_rule in conf_line_set)
+        mock_get_utility().add_message.assert_called_once_with(mock.ANY,
+                                                               mock.ANY)
+
 
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/apache2.conf

@@ -0,0 +1,196 @@
+# This is the main Apache server configuration file.  It contains the
+# configuration directives that give the server its instructions.
+# See http://httpd.apache.org/docs/2.4/ for detailed information about
+# the directives and /usr/share/doc/apache2/README.Debian about Debian specific
+# hints.
+#
+#
+# Summary of how the Apache 2 configuration works in Debian:
+# The Apache 2 web server configuration in Debian is quite different to
+# upstream's suggested way to configure the web server. This is because Debian's
+# default Apache2 installation attempts to make adding and removing modules,
+# virtual hosts, and extra configuration directives as flexible as possible, in
+# order to make automating the changes and administering the server as easy as
+# possible.
+
+# It is split into several files forming the configuration hierarchy outlined
+# below, all located in the /etc/apache2/ directory:
+#
+#	/etc/apache2/
+#	|-- apache2.conf
+#	|	`--  ports.conf
+#	|-- mods-enabled
+#	|	|-- *.load
+#	|	`-- *.conf
+#	|-- conf-enabled
+#	|	`-- *.conf
+# 	`-- sites-enabled
+#	 	`-- *.conf
+#
+#
+# * apache2.conf is the main configuration file (this file). It puts the pieces
+#   together by including all remaining configuration files when starting up the
+#   web server.
+#
+# * ports.conf is always included from the main configuration file. It is
+#   supposed to determine listening ports for incoming connections which can be
+#   customized anytime.
+#
+# * Configuration files in the mods-enabled/, conf-enabled/ and sites-enabled/
+#   directories contain particular configuration snippets which manage modules,
+#   global configuration fragments, or virtual host configurations,
+#   respectively.
+#
+#   They are activated by symlinking available configuration files from their
+#   respective *-available/ counterparts. These should be managed by using our
+#   helpers a2enmod/a2dismod, a2ensite/a2dissite and a2enconf/a2disconf. See
+#   their respective man pages for detailed information.
+#
+# * The binary is called apache2. Due to the use of environment variables, in
+#   the default configuration, apache2 needs to be started/stopped with
+#   /etc/init.d/apache2 or apache2ctl. Calling /usr/bin/apache2 directly will not
+#   work with the default configuration.
+
+
+# Global configuration
+
+#
+# The accept serialization lock file MUST BE STORED ON A LOCAL DISK.
+#
+Mutex file:${APACHE_LOCK_DIR} default
+
+#
+# PidFile: The file in which the server should record its process
+# identification number when it starts.
+# This needs to be set in /etc/apache2/envvars
+#
+PidFile ${APACHE_PID_FILE}
+
+#
+# Timeout: The number of seconds before receives and sends time out.
+#
+Timeout 300
+
+#
+# KeepAlive: Whether or not to allow persistent connections (more than
+# one request per connection). Set to "Off" to deactivate.
+#
+KeepAlive On
+
+#
+# MaxKeepAliveRequests: The maximum number of requests to allow
+# during a persistent connection. Set to 0 to allow an unlimited amount.
+# We recommend you leave this number high, for maximum performance.
+#
+MaxKeepAliveRequests 100
+
+#
+# KeepAliveTimeout: Number of seconds to wait for the next request from the
+# same client on the same connection.
+#
+KeepAliveTimeout 5
+
+
+# These need to be set in /etc/apache2/envvars
+User ${APACHE_RUN_USER}
+Group ${APACHE_RUN_GROUP}
+
+#
+# HostnameLookups: Log the names of clients or just their IP addresses
+# e.g., www.apache.org (on) or 204.62.129.132 (off).
+# The default is off because it'd be overall better for the net if people
+# had to knowingly turn this feature on, since enabling it means that
+# each client request will result in AT LEAST one lookup request to the
+# nameserver.
+#
+HostnameLookups Off
+
+# ErrorLog: The location of the error log file.
+# If you do not specify an ErrorLog directive within a <VirtualHost>
+# container, error messages relating to that virtual host will be
+# logged here.  If you *do* define an error logfile for a <VirtualHost>
+# container, that host's errors will be logged there and not here.
+#
+ErrorLog ${APACHE_LOG_DIR}/error.log
+
+#
+# LogLevel: Control the severity of messages logged to the error_log.
+# Available values: trace8, ..., trace1, debug, info, notice, warn,
+# error, crit, alert, emerg.
+# It is also possible to configure the log level for particular modules, e.g.
+# "LogLevel info ssl:warn"
+#
+LogLevel warn
+
+# Include module configuration:
+IncludeOptional mods-enabled/*.load
+IncludeOptional mods-enabled/*.conf
+
+# Include list of ports to listen on
+Include ports.conf
+
+
+# Sets the default security model of the Apache2 HTTPD server. It does
+# not allow access to the root filesystem outside of /usr/share and /var/www.
+# The former is used by web applications packaged in Debian,
+# the latter may be used for local directories served by the web server. If
+# your system is serving content from a sub-directory in /srv you must allow
+# access here, or in any related virtual host.
+<Directory />
+	Options FollowSymLinks
+	AllowOverride None
+	Require all denied
+</Directory>
+
+<Directory /usr/share>
+	AllowOverride None
+	Require all granted
+</Directory>
+
+<Directory /var/>
+	Options Indexes FollowSymLinks
+	AllowOverride None
+	Require all granted
+</Directory>
+
+# AccessFileName: The name of the file to look for in each directory
+# for additional configuration directives.  See also the AllowOverride
+# directive.
+#
+AccessFileName .htaccess
+
+#
+# The following lines prevent .htaccess and .htpasswd files from being
+# viewed by Web clients.
+#
+<FilesMatch "^\.ht">
+	Require all denied
+</FilesMatch>
+
+# The following directives define some format nicknames for use with
+# a CustomLog directive.
+#
+# These deviate from the Common Log Format definitions in that they use %O
+# (the actual bytes sent including headers) instead of %b (the size of the
+# requested file), because the latter makes it impossible to detect partial
+# requests.
+#
+# Note that the use of %{X-Forwarded-For}i instead of %h is not recommended.
+# Use mod_remoteip instead.
+#
+LogFormat "%v:%p %h %l %u %t \"%r\" %>s %O \"%{Referer}i\" \"%{User-Agent}i\"" vhost_combined
+LogFormat "%h %l %u %t \"%r\" %>s %O \"%{Referer}i\" \"%{User-Agent}i\"" combined
+LogFormat "%h %l %u %t \"%r\" %>s %O" common
+LogFormat "%{Referer}i -> %U" referer
+LogFormat "%{User-agent}i" agent
+
+# Include of directories ignores editors' and dpkg's backup files,
+# see README.Debian for details.
+
+# Include generic snippets of statements
+IncludeOptional conf-enabled/*.conf
+
+# Include the virtual host configurations:
+IncludeOptional sites-enabled/*.conf
+
+# vim: syntax=apache ts=4 sw=4 sts=4 sr noet

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/envvars

@@ -0,0 +1,29 @@
+# envvars - default environment variables for apache2ctl
+
+# this won't be correct after changing uid
+unset HOME
+
+# for supporting multiple apache2 instances
+if [ "${APACHE_CONFDIR##/etc/apache2-}" != "${APACHE_CONFDIR}" ] ; then
+	SUFFIX="-${APACHE_CONFDIR##/etc/apache2-}"
+else
+	SUFFIX=
+fi
+
+# Since there is no sane way to get the parsed apache2 config in scripts, some
+# settings are defined via environment variables and then used in apache2ctl,
+# /etc/init.d/apache2, /etc/logrotate.d/apache2, etc.
+export APACHE_RUN_USER=www-data
+export APACHE_RUN_GROUP=www-data
+# temporary state file location. This might be changed to /run in Wheezy+1
+export APACHE_PID_FILE=/var/run/apache2/apache2$SUFFIX.pid
+export APACHE_RUN_DIR=/var/run/apache2$SUFFIX
+export APACHE_LOCK_DIR=/var/lock/apache2$SUFFIX
+# Only /var/log/apache2 is handled by /etc/logrotate.d/apache2.
+export APACHE_LOG_DIR=/var/log/apache2$SUFFIX
+
+## The locale used by some modules like mod_dav
+export LANG=C
+
+export LANG
+

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/ports.conf

@@ -0,0 +1,15 @@
+# If you just change the port or add more ports here, you will likely also
+# have to change the VirtualHost statement in
+# /etc/apache2/sites-enabled/000-default.conf
+
+Listen 80
+
+<IfModule ssl_module>
+	Listen 443
+</IfModule>
+
+<IfModule mod_gnutls.c>
+	Listen 443
+</IfModule>
+
+# vim: syntax=apache ts=4 sw=4 sts=4 sr noet

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/default.conf

@@ -0,0 +1,22 @@
+<VirtualHost *:80>
+
+	ServerName banana.vomit.net
+	ServerAdmin webmaster@localhost
+	DocumentRoot /var/www/html
+
+	ErrorLog ${APACHE_LOG_DIR}/error.log
+	CustomLog ${APACHE_LOG_DIR}/access.log combined
+
+</VirtualHost>
+
+<VirtualHost *:80>
+
+	ServerName banana.vomit.com
+	ServerAdmin webmaster@localhost
+	DocumentRoot /var/www/html
+
+	ErrorLog ${APACHE_LOG_DIR}/error.log
+	CustomLog ${APACHE_LOG_DIR}/access.log combined
+
+</VirtualHost>
+# vim: syntax=apache ts=4 sw=4 sts=4 sr noet

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/multi-vhost.conf

@@ -0,0 +1,38 @@
+<VirtualHost *:80>
+    ServerName 1.multi.vhost.tld
+    ServerAlias first.multi.vhost.tld
+    ServerAdmin webmaster@localhost
+    DocumentRoot /var/www/html
+    ErrorLog ${APACHE_LOG_DIR}/error.log
+    CustomLog ${APACHE_LOG_DIR}/access.log combined
+</VirtualHost>
+
+<IfModule mod_ssl.c>
+    <VirtualHost *:80>
+        ServerName 2.multi.vhost.tld
+        ServerAlias second.multi.vhost.tld
+        ServerAdmin webmaster@localhost
+        DocumentRoot /var/www/html
+        ErrorLog ${APACHE_LOG_DIR}/error.log
+        CustomLog ${APACHE_LOG_DIR}/access.log combined
+RewriteEngine on
+RewriteCond %{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f
+RewriteRule ^(.*)$ b://u%{REQUEST_URI} [P,NE,L]
+RewriteCond %{HTTPS} !=on
+RewriteCond %{HTTPS} !^$
+RewriteRule ^ https://%{SERVER_NAME}%{REQUEST_URI} [L,NE,R=permanent]
+    </VirtualHost>
+</IfModule>
+
+<VirtualHost *:80>
+        ServerName 3.multi.vhost.tld
+        ServerAlias third.multi.vhost.tld
+        ServerAdmin webmaster@localhost
+        DocumentRoot /var/www/html
+        ErrorLog ${APACHE_LOG_DIR}/error.log
+        CustomLog ${APACHE_LOG_DIR}/access.log combined
+RewriteEngine on
+RewriteRule "^/secrets/(.+)" "https://new.example.com/docs/$1" [R,L]
+RewriteRule "^/docs/(.+)"  "http://new.example.com/docs/$1"  [R,L]
+</VirtualHost>
+

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/default.conf

@@ -0,0 +1 @@
+../sites-available/default.conf

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/multi-vhost.conf

@@ -0,0 +1 @@
+../sites-available/multi-vhost.conf

certbot-apache/certbot_apache/tests/util.py

@@ -164,5 +164,35 @@ def get_vh_truth(temp_dir, config_name):
                 set([obj.Addr.fromstring("10.2.3.4:443")]), True, True,
                 "ocspvhost.com")]
         return vh_truth
-
+    if config_name == "debian_apache_2_4/multi_vhosts":
+        prefix = os.path.join(
+            temp_dir, config_name, "apache2/sites-available")
+        aug_pre = "/files" + prefix
+        vh_truth = [
+            obj.VirtualHost(
+                os.path.join(prefix, "default.conf"),
+                os.path.join(aug_pre, "default.conf/VirtualHost[1]"),
+                set([obj.Addr.fromstring("*:80")]),
+                False, True, "ip-172-30-0-17"),
+            obj.VirtualHost(
+                os.path.join(prefix, "default.conf"),
+                os.path.join(aug_pre, "default.conf/VirtualHost[2]"),
+                set([obj.Addr.fromstring("*:80")]),
+                False, True, "banana.vomit.com"),
+            obj.VirtualHost(
+                os.path.join(prefix, "multi-vhost.conf"),
+                os.path.join(aug_pre, "multi-vhost.conf/VirtualHost[1]"),
+                set([obj.Addr.fromstring("*:80")]),
+                False, True, "1.multi.vhost.tld"),
+            obj.VirtualHost(
+                os.path.join(prefix, "multi-vhost.conf"),
+                os.path.join(aug_pre, "multi-vhost.conf/IfModule/VirtualHost"),
+                set([obj.Addr.fromstring("*:80")]),
+                False, True, "2.multi.vhost.tld"),
+            obj.VirtualHost(
+                os.path.join(prefix, "multi-vhost.conf"),
+                os.path.join(aug_pre, "multi-vhost.conf/VirtualHost[2]"),
+                set([obj.Addr.fromstring("*:80")]),
+                False, True, "3.multi.vhost.tld")]
+        return vh_truth
     return None  # pragma: no cover

certbot/reverter.py

@@ -491,7 +491,7 @@ class Reverter(object):
                     else:
                         logger.warning(
                             "File: %s - Could not be found to be deleted %s - "
-                            "LE probably shut down unexpectedly",
+                            "Certbot probably shut down unexpectedly",
                             os.linesep, path)
         except (IOError, OSError):
             logger.fatal(