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 @@ def check_parsing_errors(self, lens):
                     self.aug.get(path + "/message")))
                 raise errors.PluginError(msg)
 
-    # TODO: Cleanup this function
-    def save(self, title=None, temporary=False):
-        """Saves all changes to the configuration files.
+    def ensure_augeas_state(self):
+        """Makes sure that all Augeas dom changes are written to files to avoid
+        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,
-        all configuration changes made will be saved. According to the
-        function parameters. If an exception is raised, a new checkpoint
-        was not created.
+        if self.unsaved_files():
+            self.save_notes += "(autosave)"
+            self.save()
 
-        :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)
+    def unsaved_files(self):
+        """Lists files that have modified Augeas DOM but the changes have not
+        been written to the filesystem yet, used by `self.save()` and
+        ApacheConfigurator to check the file state.
 
         :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 @@ def save(self, title=None, temporary=False):
             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 @@ def add_parser_arguments(cls, add):
             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 @@ def __init__(self, *args, **kwargs):
         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 @@ def prepare(self):
                 "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 @@ def deploy_cert(self, domain, cert_path, key_path,
             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 @@ def deploy_cert(self, domain, cert_path, key_path,
             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 @@ def deploy_cert(self, domain, cert_path, key_path,
         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 @@ def _create_vhost(self, path):
         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 @@ def get_virtual_hosts(self):
                 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 @@ def make_vhost_ssl(self, nonssl_vhost):  # pylint: disable=too-many-locals
         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")
+            # The vhost was not found on the currently parsed paths
+            # 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 @@ def make_vhost_ssl(self, nonssl_vhost):  # pylint: disable=too-many-locals
         # 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 @@ def _get_new_vh_path(self, orig_matches, new_matches):
         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"):
-            return non_ssl_vh_fp[:-(len(".conf"))] + self.conf("le_vhost_ext")
+        """ Get a file path for SSL vhost, uses user defined path as priority,
+        but if the value is invalid or not defined, will fall back to non-ssl
+        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 @@ def _copy_create_ssl_vhost_skeleton(self, vhost, ssl_fp):
                 # 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 @@ def _write_out_redirect(self, ssl_vhost, text):
             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 @@ def _write_out_redirect(self, ssl_vhost, text):
         # 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 @@ def _get_proposed_addrs(self, vhost, port="80"):
 
         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 @@ def enable_site(self, vhost):
             supported.
 
         """
-        if self.is_site_enabled(vhost.filep):
+        if vhost.enabled:
+            return
+
+        # Handle non-debian systems
+        if not self.conf("handle-sites"):
+            if not self.parser.parsed_in_original(vhost.filep):
+                # Add direct include to root conf
+                self.parser.add_include(self.parser.loc["default"], vhost.filep)
+                vhost.enabled = True
             return
 
-        if "/sites-available/" in vhost.filep:
-            enabled_path = ("%s/sites-enabled/%s" %
-                            (self.parser.root, os.path.basename(vhost.filep)))
-            self.reverter.register_file_creation(False, enabled_path)
+        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
-            logger.info("Enabling available site: %s", vhost.filep)
-            self.save_notes += "Enabled site %s\n" % vhost.filep
-        else:
-            raise errors.NotSupportedError(
-                "Unsupported filesystem layout. "
-                "sites-available/enabled expected.")
+        except OSError as err:
+            if os.path.islink(enabled_path) and os.path.realpath(
+               enabled_path) == vhost.filep:
+                # Already in shape
+                vhost.enabled = True
+                return
+            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.