Do not parse disabled configuration files from under sites-available on Debian / Ubuntu (#4104)

This changes the apache plugin behaviour to only parse enabled configuration files and respecting the --apache-vhost-root CLI parameter for new SSL vhost creation. If --apache-vhost-root isn't defined, or doesn't exist, the SSL vhost will be created to originating non-SSL vhost directory.

This PR also implements actual check for vhost enabled state, and makes sure parser.parse_file() does not discard changes in Augeas DOM, by doing an autosave.

Also handles enabling the new SSL vhost, if it's on a path that's not parsed by Apache.

Fixes: #1328
Fixes: #3545
Fixes: #3791
Fixes: #4523
Fixes: #4837
Fixes: #4905

* First changes

* Handle rest of the errors

* Test fixes

* Final fixes

* Make parse_files accessible and fix linter problems

* Activate vhost at later time

* Cleanup

* Add a new test case, and fix old

* Enable site later in deploy_cert

* Make apache-conf-test default dummy configuration enabled

* Remove is_sites_available as obsolete

* Cleanup

* Brought back conditional vhost_path parsing

* Parenthesis

* Fix merge leftovers

* Fix to work with the recent changes to new file creation

* Added fix and tests for non-symlink vhost in sites-enabled

* Made vhostroot parameter for ApacheParser optional, and removed extra_path

* Respect vhost-root, and add Include statements to root configuration if needed

* Fixed site enabling order to prevent apache restart error while enabling mod_ssl

* Don't exclude Ubuntu / Debian vhost-root cli argument

* Changed the SSL vhost directory selection priority

* Requested fixes for paths and vhost discovery

* Make sure the Augeas DOM is written to disk before loading new files

* Actual checking for if the file is parsed within existing Apache configuration

* Fix the order of dummy SSL directives addition and enabling modules

* Restructured site_enabled checks

* Enabling vhost correctly for non-debian systems

certbot-apache/certbot_apache/augeas_configurator.py

@@ -76,26 +76,26 @@ class AugeasConfigurator(common.Installer):
                     self.aug.get(path + "/message")))
                 raise errors.PluginError(msg)
 
-    # TODO: Cleanup this function
+    def ensure_augeas_state(self):
-    def save(self, title=None, temporary=False):
+        """Makes sure that all Augeas dom changes are written to files to avoid
-        """Saves all changes to the configuration files.
+        loss of configuration directives when doing additional augeas parsing,
+        causing a possible augeas.load() resulting dom reset
+        """
 
-        This function first checks for save errors, if none are found,
+        if self.unsaved_files():
-        all configuration changes made will be saved. According to the
+            self.save_notes += "(autosave)"
-        function parameters. If an exception is raised, a new checkpoint
+            self.save()
-        was not created.
 
-        :param str title: The title of the save. If a title is given, the
+    def unsaved_files(self):
-            configuration will be saved as a new checkpoint and put in a
+        """Lists files that have modified Augeas DOM but the changes have not
-            timestamped directory.
+        been written to the filesystem yet, used by `self.save()` and
-
+        ApacheConfigurator to check the file state.
-        :param bool temporary: Indicates whether the changes made will
-            be quickly reversed in the future (ie. challenges)
 
         :raises .errors.PluginError: If there was an error in Augeas, in
             an attempt to save the configuration, or an error creating a
             checkpoint
 
+        :returns: `set` of unsaved files
         """
         save_state = self.aug.get("/augeas/save")
         self.aug.set("/augeas/save", "noop")
@@ -111,21 +111,41 @@ class AugeasConfigurator(common.Installer):
             raise errors.PluginError(
                 "Error saving files, check logs for more info.")
 
+        # Return the original save method
+        self.aug.set("/augeas/save", save_state)
+
         # Retrieve list of modified files
         # Note: Noop saves can cause the file to be listed twice, I used a
         # set to remove this possibility. This is a known augeas 0.10 error.
         save_paths = self.aug.match("/augeas/events/saved")
 
-        # If the augeas tree didn't change, no files were saved and a backup
-        # should not be created
         save_files = set()
         if save_paths:
             for path in save_paths:
                 save_files.add(self.aug.get(path)[6:])
+        return save_files
+
+    def save(self, title=None, temporary=False):
+        """Saves all changes to the configuration files.
+
+        This function first checks for save errors, if none are found,
+        all configuration changes made will be saved. According to the
+        function parameters. If an exception is raised, a new checkpoint
+        was not created.
+
+        :param str title: The title of the save. If a title is given, the
+            configuration will be saved as a new checkpoint and put in a
+            timestamped directory.
+
+        :param bool temporary: Indicates whether the changes made will
+            be quickly reversed in the future (ie. challenges)
+
+        """
+        save_files = self.unsaved_files()
+        if save_files:
             self.add_to_checkpoint(save_files,
                                    self.save_notes, temporary=temporary)
 
-        self.aug.set("/augeas/save", save_state)
         self.save_notes = ""
         self.aug.save()
 

certbot-apache/certbot_apache/configurator.py

@@ -1,6 +1,5 @@
 """Apache Configuration based off of Augeas Configurator."""
 # pylint: disable=too-many-lines
-import filecmp
 import fnmatch
 import logging
 import os
@@ -96,7 +95,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             help="SSL vhost configuration extension.")
         add("server-root", default=constants.os_constant("server_root"),
             help="Apache server root directory.")
-        add("vhost-root", default=constants.os_constant("vhost_root"),
+        add("vhost-root", default=None,
             help="Apache server VirtualHost configuration root")
         add("logs-root", default=constants.os_constant("logs_root"),
             help="Apache server logs directory")
@@ -134,6 +133,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         self.parser = None
         self.version = version
         self.vhosts = None
+        self.vhostroot = None
         self._enhance_func = {"redirect": self._enable_redirect,
                               "ensure-http-header": self._set_http_header,
                               "staple-ocsp": self._enable_ocsp_stapling}
@@ -190,9 +190,15 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 "version 1.2.0 or higher, please make sure you have you have "
                 "those installed.")
 
+        # Parse vhost-root if defined on cli
+        if not self.conf("vhost-root"):
+            self.vhostroot = constants.os_constant("vhost_root")
+        else:
+            self.vhostroot = os.path.abspath(self.conf("vhost-root"))
+
         self.parser = parser.ApacheParser(
             self.aug, self.conf("server-root"), self.conf("vhost-root"),
-            self.version)
+            self.version, configurator=self)
         # Check for errors in parsing files with Augeas
         self.check_parsing_errors("httpd.aug")
 
@@ -242,13 +248,18 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             a lack of directives
 
         """
+        # Choose vhost before (possible) enabling of mod_ssl, to keep the
+        # vhost choice namespace similar with the pre-validation one.
         vhost = self.choose_vhost(domain)
-        self._clean_vhost(vhost)
 
         # 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")
 
+        # Add directives and remove duplicates
+        self._add_dummy_ssl_directives(vhost.path)
+        self._clean_vhost(vhost)
+
         path = {"cert_path": self.parser.find_dir("SSLCertificateFile",
                                                   None, vhost.path),
                 "cert_key": self.parser.find_dir("SSLCertificateKeyFile",
@@ -290,6 +301,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             self.aug.set(path["cert_path"][-1], fullchain_path)
             self.aug.set(path["cert_key"][-1], key_path)
 
+        # Enable the new vhost if needed
+        if not vhost.enabled:
+            self.enable_site(vhost)
+
         # Save notes about the transaction that took place
         self.save_notes += ("Changed vhost at %s with addresses of %s\n"
                             "\tSSLCertificateFile %s\n"
@@ -300,11 +315,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         if chain_path is not None:
             self.save_notes += "\tSSLCertificateChainFile %s\n" % chain_path
 
-        # Make sure vhost is enabled if distro with enabled / available
-        if self.conf("handle-sites"):
-            if not vhost.enabled:
-                self.enable_site(vhost)
-
     def choose_vhost(self, target_name, temp=False):
         """Chooses a virtual host based on the given domain name.
 
@@ -579,17 +589,14 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         if filename is None:
             return None
 
-        if self.conf("handle-sites"):
-            is_enabled = self.is_site_enabled(filename)
-        else:
-            is_enabled = True
-
         macro = False
         if "/macro/" in path.lower():
             macro = True
 
+        vhost_enabled = self.parser.parsed_in_original(filename)
+
         vhost = obj.VirtualHost(filename, path, addrs, is_ssl,
-                                is_enabled, modmacro=macro)
+                                vhost_enabled, modmacro=macro)
         self._add_servernames(vhost)
         return vhost
 
@@ -644,7 +651,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 elif internal_path not in internal_paths[realpath]:
                     internal_paths[realpath].add(internal_path)
                     vhs.append(new_vhost)
-
         return vhs
 
     def is_name_vhost(self, target_addr):
@@ -855,14 +861,22 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         vh_p = self._get_new_vh_path(orig_matches, new_matches)
 
         if not vh_p:
-            raise errors.PluginError(
+            # The vhost was not found on the currently parsed paths
-                "Could not reverse map the HTTPS VirtualHost to the original")
+            # Make Augeas aware of the new vhost
+            self.parser.parse_file(ssl_fp)
+            # Try to search again
+            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()
 
         # Log actions and create save notes
         logger.info("Created an SSL vhost at %s", ssl_fp)
@@ -873,6 +887,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # 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
@@ -901,11 +916,29 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         return None
 
     def _get_ssl_vhost_path(self, non_ssl_vh_fp):
-        # Get filepath of new ssl_vhost
+        """ Get a file path for SSL vhost, uses user defined path as priority,
-        if non_ssl_vh_fp.endswith(".conf"):
+        but if the value is invalid or not defined, will fall back to non-ssl
-            return non_ssl_vh_fp[:-(len(".conf"))] + self.conf("le_vhost_ext")
+        vhost filepath.
+
+        :param str non_ssl_vh_fp: Filepath of non-SSL vhost
+
+        :returns: Filepath for SSL vhost
+        :rtype: str
+        """
+
+        if self.conf("vhost-root") and os.path.exists(self.conf("vhost-root")):
+            # Defined by user on CLI
+
+            fp = os.path.join(os.path.realpath(self.vhostroot),
+                              os.path.basename(non_ssl_vh_fp))
         else:
-            return non_ssl_vh_fp + self.conf("le_vhost_ext")
+            # Use non-ssl filepath
+            fp = os.path.realpath(non_ssl_vh_fp)
+
+        if fp.endswith(".conf"):
+            return fp[:-(len(".conf"))] + self.conf("le_vhost_ext")
+        else:
+            return fp + self.conf("le_vhost_ext")
 
     def _sift_rewrite_rule(self, line):
         """Decides whether a line should be copied to a SSL vhost.
@@ -970,6 +1003,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 # The content does not include the closing tag, so add it
                 new_file.write("</VirtualHost>\n")
                 new_file.write("</IfModule>\n")
+            # Add new file to augeas paths if we're supposed to handle
+            # activation (it's not included as default)
+            if not self.parser.parsed_in_current(ssl_fp):
+                self.parser.parse_file(ssl_fp)
         except IOError:
             logger.fatal("Error writing/reading to file in make_vhost_ssl")
             raise errors.PluginError("Unable to write/read in make_vhost_ssl")
@@ -1610,7 +1647,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             if len(ssl_vhost.name) < (255 - (len(redirect_filename) + 1)):
                 redirect_filename = "le-redirect-%s.conf" % ssl_vhost.name
 
-        redirect_filepath = os.path.join(self.conf("vhost-root"),
+        redirect_filepath = os.path.join(self.vhostroot,
                                          redirect_filename)
 
         # Register the new file that will be created
@@ -1621,6 +1658,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # Write out file
         with open(redirect_filepath, "w") as redirect_file:
             redirect_file.write(text)
+
+        # Add new include to configuration if it doesn't exist yet
+        if not self.parser.parsed_in_current(redirect_filepath):
+            self.parser.parse_file(redirect_filepath)
+
         logger.info("Created redirect file: %s", redirect_filename)
 
         return redirect_filepath
@@ -1660,32 +1702,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         return redirects
 
-    def is_site_enabled(self, avail_fp):
-        """Checks to see if the given site is enabled.
-
-        .. todo:: fix hardcoded sites-enabled, check os.path.samefile
-
-        :param str avail_fp: Complete file path of available site
-
-        :returns: Success
-        :rtype: bool
-
-        """
-
-        enabled_dir = os.path.join(self.parser.root, "sites-enabled")
-        if not os.path.isdir(enabled_dir):
-            error_msg = ("Directory '{0}' does not exist. Please ensure "
-                         "that the values for --apache-handle-sites and "
-                         "--apache-server-root are correct for your "
-                         "environment.".format(enabled_dir))
-            raise errors.ConfigurationError(error_msg)
-        for entry in os.listdir(enabled_dir):
-            try:
-                if filecmp.cmp(avail_fp, os.path.join(enabled_dir, entry)):
-                    return True
-            except OSError:
-                pass
-        return False
 
     def enable_site(self, vhost):
         """Enables an available site, Apache reload required.
@@ -1705,21 +1721,40 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             supported.
 
         """
-        if self.is_site_enabled(vhost.filep):
+        if vhost.enabled:
             return
 
-        if "/sites-available/" in vhost.filep:
+        # Handle non-debian systems
-            enabled_path = ("%s/sites-enabled/%s" %
+        if not self.conf("handle-sites"):
-                            (self.parser.root, os.path.basename(vhost.filep)))
+            if not self.parser.parsed_in_original(vhost.filep):
-            self.reverter.register_file_creation(False, enabled_path)
+                # Add direct include to root conf
+                self.parser.add_include(self.parser.loc["default"], vhost.filep)
+                vhost.enabled = True
+            return
+
+        enabled_path = ("%s/sites-enabled/%s" %
+                        (self.parser.root, os.path.basename(vhost.filep)))
+        self.reverter.register_file_creation(False, enabled_path)
+        try:
             os.symlink(vhost.filep, enabled_path)
-            vhost.enabled = True
+        except OSError as err:
-            logger.info("Enabling available site: %s", vhost.filep)
+            if os.path.islink(enabled_path) and os.path.realpath(
-            self.save_notes += "Enabled site %s\n" % vhost.filep
+               enabled_path) == vhost.filep:
-        else:
+                # Already in shape
-            raise errors.NotSupportedError(
+                vhost.enabled = True
-                "Unsupported filesystem layout. "
+                return
-                "sites-available/enabled expected.")
+            else:
+                logger.warning(
+                    "Could not symlink %s to %s, got error: %s", enabled_path,
+                    vhost.filep, err.strerror)
+                errstring = ("Encountered error while trying to enable a " +
+                             "newly created VirtualHost located at {0} by " +
+                             "linking to it from {1}")
+                raise errors.NotSupportedError(errstring.format(vhost.filep,
+                                                                enabled_path))
+        vhost.enabled = True
+        logger.info("Enabling available site: %s", vhost.filep)
+        self.save_notes += "Enabled site %s\n" % vhost.filep
 
     def enable_mod(self, mod_name, temp=False):
         """Enables module in Apache.

certbot-apache/certbot_apache/parser.py

@@ -1,4 +1,5 @@
 """ApacheParser is a member object of the ApacheConfigurator class."""
+import copy
 import fnmatch
 import logging
 import os
@@ -30,9 +31,15 @@ class ApacheParser(object):
     arg_var_interpreter = re.compile(r"\$\{[^ \}]*}")
     fnmatch_chars = set(["*", "?", "\\", "[", "]"])
 
-    def __init__(self, aug, root, vhostroot, version=(2, 4)):
+    def __init__(self, aug, root, vhostroot=None, version=(2, 4),
+                 configurator=None):
         # Note: Order is important here.
 
+        # Needed for calling save() with reverter functionality that resides in
+        # AugeasConfigurator superclass of ApacheConfigurator. This resolves
+        # issues with aug.load() after adding new files / defines to parse tree
+        self.configurator = configurator
+
         # This uses the binary, so it can be done first.
         # https://httpd.apache.org/docs/2.4/mod/core.html#define
         # https://httpd.apache.org/docs/2.4/mod/core.html#ifdefine
@@ -46,9 +53,7 @@ class ApacheParser(object):
         # Find configuration root and make sure augeas can parse it.
         self.root = os.path.abspath(root)
         self.loc = {"root": self._find_config_root()}
-        self._parse_file(self.loc["root"])
+        self.parse_file(self.loc["root"])
-
-        self.vhostroot = os.path.abspath(vhostroot)
 
         # This problem has been fixed in Augeas 1.0
         self.standardize_excl()
@@ -62,15 +67,42 @@ class ApacheParser(object):
         # Set up rest of locations
         self.loc.update(self._set_locations())
 
-        # Must also attempt to parse virtual host root
+        self.existing_paths = copy.deepcopy(self.parser_paths)
-        self._parse_file(self.vhostroot + "/" +
+
-                         constants.os_constant("vhost_files"))
+        # Must also attempt to parse additional virtual host root
+        if vhostroot:
+            self.parse_file(os.path.abspath(vhostroot) + "/" +
+                            constants.os_constant("vhost_files"))
 
         # check to see if there were unparsed define statements
         if version < (2, 4):
             if self.find_dir("Define", exclude=False):
                 raise errors.PluginError("Error parsing runtime variables")
 
+    def add_include(self, main_config, inc_path):
+        """Add Include for a new configuration file if one does not exist
+
+        :param str main_config: file path to main Apache config file
+        :param str inc_path: path of file to include
+
+        """
+        if len(self.find_dir(case_i("Include"), inc_path)) == 0:
+            logger.debug("Adding Include %s to %s",
+                         inc_path, get_aug_path(main_config))
+            self.add_dir(
+                get_aug_path(main_config),
+                "Include", inc_path)
+
+            # Add new path to parser paths
+            new_dir = os.path.dirname(inc_path)
+            new_file = os.path.basename(inc_path)
+            if new_dir in self.existing_paths.keys():
+                # Add to existing path
+                self.existing_paths[new_dir].append(new_file)
+            else:
+                # Create a new path
+                self.existing_paths[new_dir] = [new_file]
+
     def init_modules(self):
         """Iterates on the configuration until no new modules are loaded.
 
@@ -428,9 +460,9 @@ class ApacheParser(object):
 
         # Attempts to add a transform to the file if one does not already exist
         if os.path.isdir(arg):
-            self._parse_file(os.path.join(arg, "*"))
+            self.parse_file(os.path.join(arg, "*"))
         else:
-            self._parse_file(arg)
+            self.parse_file(arg)
 
         # Argument represents an fnmatch regular expression, convert it
         # Split up the path and convert each into an Augeas accepted regex
@@ -470,7 +502,7 @@ class ApacheParser(object):
             # Since Python 3.6, it returns a different pattern like (?s:.*\.load)\Z
             return fnmatch.translate(clean_fn_match)[4:-3]
 
-    def _parse_file(self, filepath):
+    def parse_file(self, filepath):
         """Parse file with Augeas
 
         Checks to see if file_path is parsed by Augeas
@@ -480,6 +512,10 @@ class ApacheParser(object):
 
         """
         use_new, remove_old = self._check_path_actions(filepath)
+        # Ensure that we have the latest Augeas DOM state on disk before
+        # calling aug.load() which reloads the state from disk
+        if self.configurator:
+            self.configurator.ensure_augeas_state()
         # Test if augeas included file for Httpd.lens
         # Note: This works for augeas globs, ie. *.conf
         if use_new:
@@ -494,6 +530,39 @@ class ApacheParser(object):
                 self._add_httpd_transform(filepath)
                 self.aug.load()
 
+    def parsed_in_current(self, filep):
+        """Checks if the file path is parsed by current Augeas parser config
+        ie. returns True if the file is found on a path that's found in live
+        Augeas configuration.
+
+        :param str filep: Path to match
+
+        :returns: True if file is parsed in existing configuration tree
+        :rtype: bool
+        """
+        return self._parsed_by_parser_paths(filep, self.parser_paths)
+
+    def parsed_in_original(self, filep):
+        """Checks if the file path is parsed by existing Apache config.
+        ie. returns True if the file is found on a path that matches Include or
+        IncludeOptional statement in the Apache configuration.
+
+        :param str filep: Path to match
+
+        :returns: True if file is parsed in existing configuration tree
+        :rtype: bool
+        """
+        return self._parsed_by_parser_paths(filep, self.existing_paths)
+
+    def _parsed_by_parser_paths(self, filep, paths):
+        """Helper function that searches through provided paths and returns
+        True if file path is found in the set"""
+        for directory in paths.keys():
+            for filename in paths[directory]:
+                if fnmatch.fnmatch(filep, os.path.join(directory, filename)):
+                    return True
+        return False
+
     def _check_path_actions(self, filepath):
         """Determine actions to take with a new augeas path
 
@@ -622,7 +691,6 @@ class ApacheParser(object):
         for name in location:
             if os.path.isfile(os.path.join(self.root, name)):
                 return os.path.join(self.root, name)
-
         raise errors.NoInstallationError("Could not find configuration root")
 
 

certbot-apache/certbot_apache/tests/apache-conf-files/apache-conf-test

@@ -26,6 +26,7 @@ function Setup() {
     ErrorLog /tmp/error.log
     CustomLog /tmp/requests.log combined
 </VirtualHost>" | sudo tee $EA/sites-available/throwaway-example.conf >/dev/null
+        sudo ln -sf $EA/sites-available/throwaway-example.conf $EA/sites-enabled/throwaway-example.conf
     else
         TMP="/tmp/`basename \"$APPEND_APACHECONF\"`.$$"
         sudo cp -a "$APPEND_APACHECONF" "$TMP"
@@ -37,6 +38,7 @@ function Cleanup() {
     if [ "$APPEND_APACHECONF" = "" ] ; then
         sudo rm /etc/apache2/sites-{enabled,available}/"$f"
         sudo rm $EA/sites-available/throwaway-example.conf
+        sudo rm $EA/sites-enabled/throwaway-example.conf
     else
         sudo mv "$TMP" "$APPEND_APACHECONF"
     fi

certbot-apache/certbot_apache/tests/augeas_configurator_test.py

@@ -31,7 +31,7 @@ class AugeasConfiguratorTest(util.ApacheTest):
 
     def test_bad_parse(self):
         # pylint: disable=protected-access
-        self.config.parser._parse_file(os.path.join(
+        self.config.parser.parse_file(os.path.join(
             self.config.parser.root, "conf-available", "bad_conf_file.conf"))
         self.assertRaises(
             errors.PluginError, self.config.check_parsing_errors, "httpd.aug")

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -8,6 +8,7 @@ import unittest
 import mock
 # six is used in mock.patch()
 import six  # pylint: disable=unused-import
+import tempfile
 
 from acme import challenges
 
@@ -34,9 +35,20 @@ class MultipleVhostsTest(util.ApacheTest):
     def setUp(self):  # pylint: disable=arguments-differ
         super(MultipleVhostsTest, self).setUp()
 
-        self.config = util.get_apache_configurator(
+        from certbot_apache.constants import os_constant
-            self.config_path, self.vhost_path, self.config_dir, self.work_dir)
+        orig_os_constant = os_constant
-        self.config = self.mock_deploy_cert(self.config)
+        def mock_os_constant(key, vhost_path=self.vhost_path):
+            """Mock default vhost path"""
+            if key == "vhost_root":
+                return vhost_path
+            else:
+                return orig_os_constant(key)
+
+        with mock.patch("certbot_apache.constants.os_constant") as mock_c:
+            mock_c.side_effect = mock_os_constant
+            self.config = util.get_apache_configurator(
+                self.config_path, None, self.config_dir, self.work_dir)
+            self.config = self.mock_deploy_cert(self.config)
         self.vh_truth = util.get_vh_truth(
             self.temp_dir, "debian_apache_2_4/multiple_vhosts")
 
@@ -125,7 +137,8 @@ class MultipleVhostsTest(util.ApacheTest):
         mock_utility.notification = mock.MagicMock(return_value=True)
         names = self.config.get_all_names()
         self.assertEqual(names, set(
-            ["certbot.demo", "ocspvhost.com", "encryption-example.demo"]
+            ["certbot.demo", "ocspvhost.com", "encryption-example.demo",
+             "nonsym.link", "vhost.in.rootconf"]
         ))
 
     @certbot_util.patch_get_utility()
@@ -145,7 +158,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
         names = self.config.get_all_names()
         # Names get filtered, only 5 are returned
-        self.assertEqual(len(names), 5)
+        self.assertEqual(len(names), 7)
         self.assertTrue("zombo.com" in names)
         self.assertTrue("google.com" in names)
         self.assertTrue("certbot.demo" in names)
@@ -185,14 +198,9 @@ class MultipleVhostsTest(util.ApacheTest):
             self.vh_truth[2].get_names(), set(["*.le.co", "ip-172-30-0-17"]))
 
     def test_get_virtual_hosts(self):
-        """Make sure all vhosts are being properly found.
+        """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.  If finding only 3, likely is_ssl
-
-        """
         vhs = self.config.get_virtual_hosts()
-        self.assertEqual(len(vhs), 8)
+        self.assertEqual(len(vhs), 10)
         found = 0
 
         for vhost in vhs:
@@ -203,7 +211,7 @@ class MultipleVhostsTest(util.ApacheTest):
             else:
                 raise Exception("Missed: %s" % vhost)  # pragma: no cover
 
-        self.assertEqual(found, 8)
+        self.assertEqual(found, 10)
 
         # Handle case of non-debian layout get_virtual_hosts
         with mock.patch(
@@ -211,7 +219,7 @@ class MultipleVhostsTest(util.ApacheTest):
         ) as mock_conf:
             mock_conf.return_value = False
             vhs = self.config.get_virtual_hosts()
-            self.assertEqual(len(vhs), 8)
+            self.assertEqual(len(vhs), 10)
 
     @mock.patch("certbot_apache.display_ops.select_vhost")
     def test_choose_vhost_none_avail(self, mock_select):
@@ -226,8 +234,10 @@ class MultipleVhostsTest(util.ApacheTest):
             self.vh_truth[1], self.config.choose_vhost("none.com"))
 
     @mock.patch("certbot_apache.display_ops.select_vhost")
-    def test_choose_vhost_select_vhost_non_ssl(self, mock_select):
+    @mock.patch("certbot_apache.obj.VirtualHost.conflicts")
+    def test_choose_vhost_select_vhost_non_ssl(self, mock_conf, mock_select):
         mock_select.return_value = self.vh_truth[0]
+        mock_conf.return_value = False
         chosen_vhost = self.config.choose_vhost("none.com")
         self.vh_truth[0].aliases.add("none.com")
         self.assertEqual(
@@ -237,6 +247,15 @@ class MultipleVhostsTest(util.ApacheTest):
         self.assertFalse(self.vh_truth[0].ssl)
         self.assertTrue(chosen_vhost.ssl)
 
+    @mock.patch("certbot_apache.configurator.ApacheConfigurator._find_best_vhost")
+    @mock.patch("certbot_apache.parser.ApacheParser.add_dir")
+    def test_choose_vhost_and_servername_addition(self, mock_add, mock_find):
+        ret_vh = self.vh_truth[8]
+        ret_vh.enabled = False
+        mock_find.return_value = self.vh_truth[8]
+        self.config.choose_vhost("whatever.com")
+        self.assertTrue(mock_add.called)
+
     @mock.patch("certbot_apache.display_ops.select_vhost")
     def test_choose_vhost_select_vhost_with_temp(self, mock_select):
         mock_select.return_value = self.vh_truth[0]
@@ -288,9 +307,9 @@ class MultipleVhostsTest(util.ApacheTest):
         # Assume only the two default vhosts.
         self.config.vhosts = [
             vh for vh in self.config.vhosts
-            if vh.name not in ["certbot.demo",
+            if vh.name not in ["certbot.demo", "nonsym.link",
                 "encryption-example.demo",
-                "ocspvhost.com"]
+                "ocspvhost.com", "vhost.in.rootconf"]
             and "*.blue.purple.com" not in vh.aliases
         ]
         self.assertEqual(
@@ -299,26 +318,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
     def test_non_default_vhosts(self):
         # pylint: disable=protected-access
-        self.assertEqual(len(self.config._non_default_vhosts()), 6)
+        self.assertEqual(len(self.config._non_default_vhosts()), 8)
-
-    def test_is_site_enabled(self):
-        """Test if site is enabled.
-
-        .. note:: This test currently fails for hard links
-            (which may happen if you move dirs incorrectly)
-        .. warning:: This test does not work when running using the
-            unittest.main() function. It incorrectly copies symlinks.
-
-        """
-        self.assertTrue(self.config.is_site_enabled(self.vh_truth[0].filep))
-        self.assertFalse(self.config.is_site_enabled(self.vh_truth[1].filep))
-        self.assertTrue(self.config.is_site_enabled(self.vh_truth[2].filep))
-        self.assertTrue(self.config.is_site_enabled(self.vh_truth[3].filep))
-        with mock.patch("os.path.isdir") as mock_isdir:
-            mock_isdir.return_value = False
-            self.assertRaises(errors.ConfigurationError,
-                              self.config.is_site_enabled,
-                              "irrelevant")
 
     @mock.patch("certbot.util.run_script")
     @mock.patch("certbot.util.exe_exists")
@@ -345,21 +345,59 @@ class MultipleVhostsTest(util.ApacheTest):
         self.assertRaises(
             errors.MisconfigurationError, self.config.enable_mod, "ssl")
 
-    def test_enable_site(self):
+    def test_enable_site_already_enabled(self):
-        # 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)
-
-        # Go again to make sure nothing fails
         self.config.enable_site(self.vh_truth[1])
 
     def test_enable_site_failure(self):
+        self.config.parser.root = "/tmp/nonexistent"
         self.assertRaises(
             errors.NotSupportedError,
             self.config.enable_site,
             obj.VirtualHost("asdf", "afsaf", set(), False, False))
 
+    def test_enable_site_nondebian(self):
+        mock_c = "certbot_apache.configurator.ApacheConfigurator.conf"
+        def conf_side_effect(arg):
+            """ Mock function for ApacheConfigurator.conf """
+            confvars = {"handle-sites": False}
+            if arg in confvars:
+                return confvars[arg]
+        inc_path = "/path/to/whereever"
+        vhost = self.vh_truth[0]
+        with mock.patch(mock_c) as mock_conf:
+            mock_conf.side_effect = conf_side_effect
+            vhost.enabled = False
+            vhost.filep = inc_path
+            self.assertFalse(self.config.parser.find_dir("Include", inc_path))
+            self.assertFalse(
+                os.path.dirname(inc_path) in self.config.parser.existing_paths)
+            self.config.enable_site(vhost)
+            self.assertTrue(self.config.parser.find_dir("Include", inc_path))
+            self.assertTrue(
+                os.path.dirname(inc_path) in self.config.parser.existing_paths)
+            self.assertTrue(
+                os.path.basename(inc_path) in self.config.parser.existing_paths[
+                    os.path.dirname(inc_path)])
+
+    def test_deploy_cert_enable_new_vhost(self):
+        # Create
+        ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0])
+        self.config.parser.modules.add("ssl_module")
+        self.config.parser.modules.add("mod_ssl.c")
+        self.assertFalse(ssl_vhost.enabled)
+        self.config.deploy_cert(
+            "encryption-example.demo", "example/cert.pem", "example/key.pem",
+            "example/cert_chain.pem", "example/fullchain.pem")
+        self.assertTrue(ssl_vhost.enabled)
+        # Make sure that we don't error out if symlink already exists
+        ssl_vhost.enabled = False
+        self.assertFalse(ssl_vhost.enabled)
+        self.config.deploy_cert(
+            "encryption-example.demo", "example/cert.pem", "example/key.pem",
+            "example/cert_chain.pem", "example/fullchain.pem")
+        self.assertTrue(ssl_vhost.enabled)
+
     def test_deploy_cert_newssl(self):
         self.config = util.get_apache_configurator(
             self.config_path, self.vhost_path, self.config_dir,
@@ -388,12 +426,14 @@ class MultipleVhostsTest(util.ApacheTest):
 
         # Verify one directive was found in the correct file
         self.assertEqual(len(loc_cert), 1)
-        self.assertEqual(configurator.get_file_path(loc_cert[0]),
+        self.assertEqual(
-                         self.vh_truth[1].filep)
+            configurator.get_file_path(loc_cert[0]),
+            self.vh_truth[1].filep)
 
         self.assertEqual(len(loc_key), 1)
-        self.assertEqual(configurator.get_file_path(loc_key[0]),
+        self.assertEqual(
-                         self.vh_truth[1].filep)
+            configurator.get_file_path(loc_key[0]),
+            self.vh_truth[1].filep)
 
     def test_deploy_cert_newssl_no_fullchain(self):
         self.config = util.get_apache_configurator(
@@ -427,10 +467,75 @@ class MultipleVhostsTest(util.ApacheTest):
                               "random.demo", "example/cert.pem",
                               "example/key.pem"))
 
+    def test_deploy_cert_not_parsed_path(self):
+        # Make sure that we add include to root config for vhosts when
+        # handle-sites is false
+        self.config.parser.modules.add("ssl_module")
+        self.config.parser.modules.add("mod_ssl.c")
+        tmp_path = os.path.realpath(tempfile.mkdtemp("vhostroot"))
+        os.chmod(tmp_path, 0o755)
+        mock_p = "certbot_apache.configurator.ApacheConfigurator._get_ssl_vhost_path"
+        mock_a = "certbot_apache.parser.ApacheParser.add_include"
+        mock_c = "certbot_apache.configurator.ApacheConfigurator.conf"
+        orig_conf = self.config.conf
+        def conf_side_effect(arg):
+            """ Mock function for ApacheConfigurator.conf """
+            confvars = {"handle-sites": False}
+            if arg in confvars:
+                return confvars[arg]
+            else:
+                return orig_conf("arg")
+
+        with mock.patch(mock_c) as mock_conf:
+            mock_conf.side_effect = conf_side_effect
+            with mock.patch(mock_p) as mock_path:
+                mock_path.return_value = os.path.join(tmp_path, "whatever.conf")
+                with mock.patch(mock_a) as mock_add:
+                    self.config.deploy_cert(
+                        "encryption-example.demo",
+                        "example/cert.pem", "example/key.pem",
+                        "example/cert_chain.pem")
+                    # Test that we actually called add_include
+                    self.assertTrue(mock_add.called)
+        shutil.rmtree(tmp_path)
+
+
     def test_deploy_cert(self):
         self.config.parser.modules.add("ssl_module")
         self.config.parser.modules.add("mod_ssl.c")
 
+        # Patch _add_dummy_ssl_directives to make sure we write them correctly
+        # pylint: disable=protected-access
+        orig_add_dummy = self.config._add_dummy_ssl_directives
+        def mock_add_dummy_ssl(vhostpath):
+            """Mock method for _add_dummy_ssl_directives"""
+            def find_args(path, directive):
+                """Return list of arguments in requested directive at path"""
+                f_args = []
+                dirs = self.config.parser.find_dir(directive, None,
+                                                   path)
+                for d in dirs:
+                    f_args.append(self.config.parser.get_arg(d))
+                return f_args
+            # Verify that the dummy directives do not exist
+            self.assertFalse(
+                "insert_cert_file_path" in find_args(vhostpath,
+                                                     "SSLCertificateFile"))
+            self.assertFalse(
+                "insert_key_file_path" in find_args(vhostpath,
+                                                    "SSLCertificateKeyFile"))
+            orig_add_dummy(vhostpath)
+            # Verify that the dummy directives exist
+            self.assertTrue(
+                "insert_cert_file_path" in find_args(vhostpath,
+                                                     "SSLCertificateFile"))
+            self.assertTrue(
+                "insert_key_file_path" in find_args(vhostpath,
+                                                    "SSLCertificateKeyFile"))
+
+        # pylint: disable=protected-access
+        self.config._add_dummy_ssl_directives = mock_add_dummy_ssl
+
         # Get the default 443 vhost
         self.config.assoc["random.demo"] = self.vh_truth[1]
         self.config.deploy_cert(
@@ -452,16 +557,19 @@ class MultipleVhostsTest(util.ApacheTest):
 
         # Verify one directive was found in the correct file
         self.assertEqual(len(loc_cert), 1)
-        self.assertEqual(configurator.get_file_path(loc_cert[0]),
+        self.assertEqual(
-                         self.vh_truth[1].filep)
+            configurator.get_file_path(loc_cert[0]),
+            self.vh_truth[1].filep)
 
         self.assertEqual(len(loc_key), 1)
-        self.assertEqual(configurator.get_file_path(loc_key[0]),
+        self.assertEqual(
-                         self.vh_truth[1].filep)
+            configurator.get_file_path(loc_key[0]),
+            self.vh_truth[1].filep)
 
         self.assertEqual(len(loc_chain), 1)
-        self.assertEqual(configurator.get_file_path(loc_chain[0]),
+        self.assertEqual(
-                         self.vh_truth[1].filep)
+            configurator.get_file_path(loc_chain[0]),
+            self.vh_truth[1].filep)
 
         # One more time for chain directive setting
         self.config.deploy_cert(
@@ -614,6 +722,30 @@ class MultipleVhostsTest(util.ApacheTest):
             mock_span.return_value = return_value
             self.test_make_vhost_ssl()
 
+    def test_make_vhost_ssl_nonsymlink(self):
+        ssl_vhost_slink = self.config.make_vhost_ssl(self.vh_truth[8])
+        self.assertTrue(ssl_vhost_slink.ssl)
+        self.assertTrue(ssl_vhost_slink.enabled)
+        self.assertEqual(ssl_vhost_slink.name, "nonsym.link")
+
+    def test_make_vhost_ssl_nonexistent_vhost_path(self):
+        def conf_side_effect(arg):
+            """ Mock function for ApacheConfigurator.conf """
+            confvars = {
+                "vhost-root": "/tmp/nonexistent",
+                "le_vhost_ext": "-le-ssl.conf",
+                "handle-sites": True}
+            return confvars[arg]
+
+        with mock.patch(
+                "certbot_apache.configurator.ApacheConfigurator.conf"
+        ) as mock_conf:
+            mock_conf.side_effect = conf_side_effect
+            ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[1])
+            self.assertEqual(os.path.dirname(ssl_vhost.filep),
+                             os.path.dirname(os.path.realpath(
+                                 self.vh_truth[1].filep)))
+
     def test_make_vhost_ssl(self):
         ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0])
 
@@ -630,15 +762,10 @@ class MultipleVhostsTest(util.ApacheTest):
         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[0]),
                          self.config.is_name_vhost(ssl_vhost))
 
-        self.assertEqual(len(self.config.vhosts), 9)
+        self.assertEqual(len(self.config.vhosts), 11)
 
     def test_clean_vhost_ssl(self):
         # pylint: disable=protected-access
@@ -688,17 +815,17 @@ class MultipleVhostsTest(util.ApacheTest):
         DIRECTIVES = ["Foo", "Bar"]
         for directive in DIRECTIVES:
             for _ in range(10):
-                self.config.parser.add_dir(self.vh_truth[1].path,
+                self.config.parser.add_dir(self.vh_truth[2].path,
                                            directive, ["baz"])
         self.config.save()
 
-        self.config._remove_directives(self.vh_truth[1].path, DIRECTIVES)
+        self.config._remove_directives(self.vh_truth[2].path, DIRECTIVES)
         self.config.save()
 
         for directive in DIRECTIVES:
             self.assertEqual(
                 len(self.config.parser.find_dir(
-                    directive, None, self.vh_truth[1].path, False)), 0)
+                    directive, None, self.vh_truth[2].path, False)), 0)
 
     def test_make_vhost_ssl_bad_write(self):
         mock_open = mock.mock_open()
@@ -717,10 +844,10 @@ class MultipleVhostsTest(util.ApacheTest):
 
     def test_add_name_vhost_if_necessary(self):
         # pylint: disable=protected-access
-        self.config.save = mock.Mock()
+        self.config.add_name_vhost = mock.Mock()
         self.config.version = (2, 2)
         self.config._add_name_vhost_if_necessary(self.vh_truth[0])
-        self.assertTrue(self.config.save.called)
+        self.assertTrue(self.config.add_name_vhost.called)
 
         new_addrs = set()
         for addr in self.vh_truth[0].addrs:
@@ -728,7 +855,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
         self.vh_truth[0].addrs = new_addrs
         self.config._add_name_vhost_if_necessary(self.vh_truth[0])
-        self.assertEqual(self.config.save.call_count, 2)
+        self.assertEqual(self.config.add_name_vhost.call_count, 2)
 
     @mock.patch("certbot_apache.configurator.tls_sni_01.ApacheTlsSni01.perform")
     @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart")
@@ -915,7 +1042,6 @@ class MultipleVhostsTest(util.ApacheTest):
             "SSLUseStapling", "on", ssl_vhost.path)
 
         self.assertEqual(len(ssl_use_stapling_aug_path), 1)
-
         ssl_vhost_aug_path = parser.get_aug_path(ssl_vhost.filep)
         stapling_cache_aug_path = self.config.parser.find_dir('SSLStaplingCache',
                     "shmcb:/var/run/apache2/stapling_cache(128000)",
@@ -1177,7 +1303,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
         # pylint: disable=protected-access
         self.config._enable_redirect(self.vh_truth[1], "")
-        self.assertEqual(len(self.config.vhosts), 9)
+        self.assertEqual(len(self.config.vhosts), 11)
 
     def test_create_own_redirect_for_old_apache_version(self):
         self.config.parser.modules.add("rewrite_module")
@@ -1188,7 +1314,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
         # pylint: disable=protected-access
         self.config._enable_redirect(self.vh_truth[1], "")
-        self.assertEqual(len(self.config.vhosts), 9)
+        self.assertEqual(len(self.config.vhosts), 11)
 
     def test_sift_rewrite_rule(self):
         # pylint: disable=protected-access
@@ -1285,13 +1411,17 @@ class AugeasVhostsTest(util.ApacheTest):
         for name in names:
             self.assertFalse(name in self.config.choose_vhost(name).aliases)
 
-    def test_choose_vhost_without_matching_wildcard(self):
+    @mock.patch("certbot_apache.obj.VirtualHost.conflicts")
+    def test_choose_vhost_without_matching_wildcard(self, mock_conflicts):
+        mock_conflicts.return_value = False
         mock_path = "certbot_apache.display_ops.select_vhost"
         with mock.patch(mock_path, lambda _, vhosts: vhosts[0]):
             for name in ("a.example.net", "other.example.net"):
                 self.assertTrue(name in self.config.choose_vhost(name).aliases)
 
-    def test_choose_vhost_wildcard_not_found(self):
+    @mock.patch("certbot_apache.obj.VirtualHost.conflicts")
+    def test_choose_vhost_wildcard_not_found(self, mock_conflicts):
+        mock_conflicts.return_value = False
         mock_path = "certbot_apache.display_ops.select_vhost"
         names = (
             "abc.example.net", "not.there.tld", "aa.wildcard.tld"
@@ -1358,10 +1488,6 @@ class MultiVhostsTest(util.ApacheTest):
         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))

certbot-apache/certbot_apache/tests/parser_test.py

@@ -38,7 +38,7 @@ class BasicParserTest(util.ParserTest):
         file_path = os.path.join(
             self.config_path, "not-parsed-by-default", "certbot.conf")
 
-        self.parser._parse_file(file_path)  # pylint: disable=protected-access
+        self.parser.parse_file(file_path)  # pylint: disable=protected-access
 
         # search for the httpd incl
         matches = self.parser.aug.match(
@@ -52,7 +52,7 @@ class BasicParserTest(util.ParserTest):
         test2 = self.parser.find_dir("documentroot")
 
         self.assertEqual(len(test), 1)
-        self.assertEqual(len(test2), 4)
+        self.assertEqual(len(test2), 7)
 
     def test_add_dir(self):
         aug_default = "/files" + self.parser.loc["default"]

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-enabled/another_wildcard.conf

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

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-enabled/old,default.conf

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

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-enabled/wildcard.conf

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

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

@@ -193,4 +193,15 @@ IncludeOptional conf-enabled/*.conf
 # Include the virtual host configurations:
 IncludeOptional sites-enabled/*.conf
 
+<VirtualHost *:80>
+
+	ServerName vhost.in.rootconf
+	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/multiple_vhosts/apache2/sites-enabled/default-ssl-port-only.conf

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

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

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

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/non-symlink.conf

@@ -0,0 +1,9 @@
+<VirtualHost *:80>
+ServerName nonsym.link
+ServerAdmin webmaster@localhost
+
+DocumentRoot /var/www-certbot-reworld/static/
+
+ErrorLog ${APACHE_LOG_DIR}/error.log
+CustomLog ${APACHE_LOG_DIR}/access.log combined
+</VirtualHost>

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/wildcard.conf

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

certbot-apache/certbot_apache/tests/util.py

@@ -45,6 +45,9 @@ class ApacheTest(unittest.TestCase):  # pylint: disable=too-few-public-methods
             return
 
         for vhost_basename in os.listdir(sites_enabled):
+            # Keep the one non-symlink test vhost in place
+            if vhost_basename == "non-symlink.conf":
+                continue
             vhost = os.path.join(sites_enabled, vhost_basename)
             if not os.path.islink(vhost):  # pragma: no cover
                 os.remove(vhost)
@@ -115,7 +118,8 @@ def get_vh_truth(temp_dir, config_name):
     """Return the ground truth for the specified directory."""
     if config_name == "debian_apache_2_4/multiple_vhosts":
         prefix = os.path.join(
-            temp_dir, config_name, "apache2/sites-available")
+            temp_dir, config_name, "apache2/sites-enabled")
+
         aug_pre = "/files" + prefix
         vh_truth = [
             obj.VirtualHost(
@@ -125,8 +129,9 @@ def get_vh_truth(temp_dir, config_name):
                 False, True, "encryption-example.demo"),
             obj.VirtualHost(
                 os.path.join(prefix, "default-ssl.conf"),
-                os.path.join(aug_pre, "default-ssl.conf/IfModule/VirtualHost"),
+                os.path.join(aug_pre,
-                set([obj.Addr.fromstring("_default_:443")]), True, False),
+                             "default-ssl.conf/IfModule/VirtualHost"),
+                set([obj.Addr.fromstring("_default_:443")]), True, True),
             obj.VirtualHost(
                 os.path.join(prefix, "000-default.conf"),
                 os.path.join(aug_pre, "000-default.conf/VirtualHost"),
@@ -148,17 +153,34 @@ def get_vh_truth(temp_dir, config_name):
                 os.path.join(prefix, "default-ssl-port-only.conf"),
                 os.path.join(aug_pre, ("default-ssl-port-only.conf/"
                                        "IfModule/VirtualHost")),
-                set([obj.Addr.fromstring("_default_:443")]), True, False),
+                set([obj.Addr.fromstring("_default_:443")]), True, True),
             obj.VirtualHost(
                 os.path.join(prefix, "wildcard.conf"),
                 os.path.join(aug_pre, "wildcard.conf/VirtualHost"),
-                set([obj.Addr.fromstring("*:80")]), False, False,
+                set([obj.Addr.fromstring("*:80")]), False, True,
                 "ip-172-30-0-17", aliases=["*.blue.purple.com"]),
             obj.VirtualHost(
                 os.path.join(prefix, "ocsp-ssl.conf"),
                 os.path.join(aug_pre, "ocsp-ssl.conf/IfModule/VirtualHost"),
                 set([obj.Addr.fromstring("10.2.3.4:443")]), True, True,
-                "ocspvhost.com")]
+                "ocspvhost.com"),
+            obj.VirtualHost(
+                os.path.join(prefix, "non-symlink.conf"),
+                os.path.join(aug_pre, "non-symlink.conf/VirtualHost"),
+                set([obj.Addr.fromstring("*:80")]), False, True,
+                "nonsym.link"),
+            obj.VirtualHost(
+                os.path.join(prefix, "default-ssl-port-only.conf"),
+                os.path.join(aug_pre,
+                             "default-ssl-port-only.conf/VirtualHost"),
+                set([obj.Addr.fromstring("*:80")]), True, True, ""),
+            obj.VirtualHost(
+                os.path.join(temp_dir, config_name,
+                             "apache2/apache2.conf"),
+                "/files" + os.path.join(temp_dir, config_name,
+                                        "apache2/apache2.conf/VirtualHost"),
+                set([obj.Addr.fromstring("*:80")]), False, True,
+                "vhost.in.rootconf")]
         return vh_truth
     if config_name == "debian_apache_2_4/multi_vhosts":
         prefix = os.path.join(

certbot-apache/certbot_apache/tls_sni_01.py

@@ -7,7 +7,6 @@ from certbot.plugins import common
 from certbot.errors import PluginError, MissingCommandlineFlag
 
 from certbot_apache import obj
-from certbot_apache import parser
 
 logger = logging.getLogger(__name__)
 
@@ -105,7 +104,8 @@ class ApacheTlsSni01(common.TLSSNI01):
 
         config_text += "</IfModule>\n"
 
-        self._conf_include_check(self.configurator.parser.loc["default"])
+        self.configurator.parser.add_include(
+            self.configurator.parser.loc["default"], self.challenge_conf)
         self.configurator.reverter.register_file_creation(
             True, self.challenge_conf)
 
@@ -142,24 +142,6 @@ class ApacheTlsSni01(common.TLSSNI01):
 
         return addrs
 
-    def _conf_include_check(self, main_config):
-        """Add TLS-SNI-01 challenge conf file into configuration.
-
-        Adds TLS-SNI-01 challenge include file if it does not already exist
-        within mainConfig
-
-        :param str main_config: file path to main user apache config file
-
-        """
-        if len(self.configurator.parser.find_dir(
-                parser.case_i("Include"), self.challenge_conf)) == 0:
-            # print "Including challenge virtual host(s)"
-            logger.debug("Adding Include %s to %s",
-                         self.challenge_conf, parser.get_aug_path(main_config))
-            self.configurator.parser.add_dir(
-                parser.get_aug_path(main_config),
-                "Include", self.challenge_conf)
-
     def _get_config_text(self, achall, ip_addrs):
         """Chocolate virtual server configuration text