Handle missing vhosts gracefully

letsencrypt_apache/configurator.py

@@ -21,6 +21,7 @@ from letsencrypt import le_util
 from letsencrypt.plugins import common
 
 from letsencrypt_apache import constants
+from letsencrypt_apache import display_ops
 from letsencrypt_apache import dvsni
 from letsencrypt_apache import obj
 from letsencrypt_apache import parser
@@ -100,7 +101,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         add("le-vhost-ext", default=constants.CLI_DEFAULTS["le_vhost_ext"],
             help="SSL vhost configuration extension.")
 
-
     def __init__(self, *args, **kwargs):
         """Initialize an Apache Configurator.
 
@@ -171,7 +171,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         """
         vhost = self.choose_vhost(domain)
-        # TODO(jdkasten): vhost might be None
+
         path = {}
 
         path["cert_path"] = self.parser.find_dir(parser.case_i(
@@ -218,14 +218,16 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
     def choose_vhost(self, target_name):
         """Chooses a virtual host based on the given domain name.
 
-        .. todo:: This should maybe return list if no obvious answer
+        If there is no clear virtual host to be selected, the user is prompted
-            is presented.
+        with all available choices.
 
         :param str target_name: domain name
 
         :returns: ssl vhost associated with name
         :rtype: :class:`~letsencrypt_apache.obj.VirtualHost`
 
+        :raises .errors.ConfiguratorError: If no vhost is available
+
         """
         # Allows for domain names to be associated with a virtual host
         # Client isn't using create_dn_server_assoc(self, dn, vh) yet
@@ -251,11 +253,24 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 self.assoc[target_name] = vhost
                 return vhost
 
-        # No matches, search for the default
+        vhost = display_ops.select_vhost(target_name, self.vhosts)
-        for vhost in self.vhosts:
+        if vhost is not None:
-            if "_default_:443" in vhost.addrs:
+            self.assoc[target_name] = vhost
-                return vhost
+        else:
-        return None
+            logging.error(
+                "No vhost exists with servername or alias of: %s. "
+                "No vhost was selected. Please specify servernames "
+                "in the Apache config", target_name)
+            raise errors.ConfiguratorError("No vhost selected")
+
+        # TODO: Ask the user if they would like to add ServerName/Alias to VH
+
+        return vhost
+
+        # # No matches, search for the default
+        # for vhost in self.vhosts:
+        #     if "_default_:443" in vhost.addrs:
+        #         return vhost
 
     def create_dn_server_assoc(self, domain, vhost):
         """Create an association between a domain name and virtual host.
@@ -405,10 +420,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         is appropriately listening on port 443.
 
         """
-        if not mod_loaded("ssl_module", self.conf("ctl")):
+        if not self.mod_loaded("ssl_module"):
             logging.info("Loading mod_ssl into Apache Server")
-            enable_mod("ssl", self.conf("init-script"),
+            self.enable_mod("ssl")
-                       self.conf("enmod"))
 
         # Check for Listen 443
         # Note: This could be made to also look for ip:443 combo
@@ -462,6 +476,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         :returns: SSL vhost
         :rtype: :class:`~letsencrypt_apache.obj.VirtualHost`
 
+        :raises .errors.ConfiguratorError: If more than one virtual host is in
+            the file.
+
         """
         avail_fp = nonssl_vhost.filep
         # Get filepath of new ssl_vhost
@@ -506,7 +523,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                               (ssl_fp, parser.case_i("VirtualHost")))
         if len(vh_p) != 1:
             logging.error("Error: should only be one vhost in %s", avail_fp)
-            sys.exit(1)
+            raise errors.ConfiguratorError
 
         self.parser.add_dir(vh_p[0], "SSLCertificateFile",
                             "/etc/ssl/certs/ssl-cert-snakeoil.pem")
@@ -589,8 +606,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         :rtype: (bool, :class:`~letsencrypt_apache.obj.VirtualHost`)
 
         """
-        if not mod_loaded("rewrite_module", self.conf("ctl")):
+        if not self.mod_loaded("rewrite_module"):
-            enable_mod("rewrite", self.conf("init-script"), self.conf("enmod"))
+            self.enable_mod("rewrite")
 
         general_v = self._general_vhost(ssl_vhost)
         if general_v is None:
@@ -901,6 +918,58 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             return True
         return False
 
+    def enable_mod(self, mod_name):
+        """Enables module in Apache.
+
+        Both enables and restarts Apache so module is active.
+
+        :param str mod_name: Name of the module to enable.
+
+        """
+        try:
+            # Use check_output so the command will finish before reloading
+            # TODO: a2enmod is debian specific...
+            self.
+            subprocess.check_call([self.conf("enmod"), mod_name],
+                                  stdout=open("/dev/null", "w"),
+                                  stderr=open("/dev/null", "w"))
+            apache_restart(self.conf("init"))
+        except (OSError, subprocess.CalledProcessError):
+            logging.exception("Error enabling mod_%s", mod_name)
+            sys.exit(1)
+
+    def mod_loaded(self, module):
+        """Checks to see if mod_ssl is loaded
+
+        Uses ``apache_ctl`` to get loaded module list. This also effectively
+        serves as a config_test.
+
+        :returns: If ssl_module is included and active in Apache
+        :rtype: bool
+
+        """
+        try:
+            proc = subprocess.Popen(
+                [self.conf("ctl"), "-M"],
+                stdout=subprocess.PIPE,
+                stderr=subprocess.PIPE)
+            stdout, stderr = proc.communicate()
+
+        except (OSError, ValueError):
+            logging.error(
+                "Error accessing %s for loaded modules!", self.conf("ctl"))
+            raise errors.ConfiguratorError("Error accessing loaded modules")
+        # Small errors that do not impede
+        if proc.returncode != 0:
+            logging.warn("Error in checking loaded module list: %s", stderr)
+            raise errors.MisconfigurationError(
+                "Apache is unable to check whether or not the module is "
+                "loaded because Apache is misconfigured.")
+
+        if module in stdout:
+            return True
+        return False
+
     def restart(self):
         """Restarts apache server.
 
@@ -1040,63 +1109,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             self.restart()
 
 
-def enable_mod(mod_name, apache_init_script, apache_enmod):
-    """Enables module in Apache.
-
-    Both enables and restarts Apache so module is active.
-
-    :param str mod_name: Name of the module to enable.
-    :param str apache_init_script: Path to the Apache init script.
-    :param str apache_enmod: Path to the Apache a2enmod script.
-
-    """
-    try:
-        # Use check_output so the command will finish before reloading
-        # TODO: a2enmod is debian specific...
-        subprocess.check_call([apache_enmod, mod_name],
-                              stdout=open("/dev/null", "w"),
-                              stderr=open("/dev/null", "w"))
-        apache_restart(apache_init_script)
-    except (OSError, subprocess.CalledProcessError):
-        logging.exception("Error enabling mod_%s", mod_name)
-        sys.exit(1)
-
-
-def mod_loaded(module, apache_ctl):
-    """Checks to see if mod_ssl is loaded
-
-    Uses ``apache_ctl`` to get loaded module list. This also effectively
-    serves as a config_test.
-
-    :param str apache_ctl: Path to apache2ctl binary.
-
-    :returns: If ssl_module is included and active in Apache
-    :rtype: bool
-
-    """
-    try:
-        proc = subprocess.Popen(
-            [apache_ctl, "-M"],
-            stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE)
-        stdout, stderr = proc.communicate()
-
-    except (OSError, ValueError):
-        logging.error(
-            "Error accessing %s for loaded modules!", apache_ctl)
-        raise errors.ConfiguratorError("Error accessing loaded modules")
-    # Small errors that do not impede
-    if proc.returncode != 0:
-        logging.warn("Error in checking loaded module list: %s", stderr)
-        raise errors.MisconfigurationError(
-            "Apache is unable to check whether or not the module is "
-            "loaded because Apache is misconfigured.")
-
-    if module in stdout:
-        return True
-    return False
-
-
 def apache_restart(apache_init_script):
     """Restarts the Apache Server.
 

letsencrypt_apache/display_ops.py

@@ -0,0 +1,70 @@
+import os
+import zope.component
+
+from letsencrypt import interfaces
+
+import letsencrypt.display.util as display_util
+
+
+def select_vhost(domain, vhosts):
+    """Select an appropriate Apache Vhost.
+
+    :param vhosts: Available Apache Virtual Hosts
+    :type vhosts: :class:`list` of type `~obj.Vhost`
+
+    :returns: VirtualHost
+    :rtype: `~obj.Vhost`
+
+    """
+    if not vhosts:
+        return None
+    while True:
+        code, tag = _vhost_menu(domain, vhosts)
+        if code == display_util.HELP:
+            _more_info_vhost(vhosts[tag])
+        elif code == display_util.OK:
+            return vhosts[tag]
+        else:
+            return None
+
+
+def _vhost_menu(domain, vhosts):
+    """Select an appropriate Apache Vhost.
+
+    :param vhosts: Available Apache Virtual Hosts
+    :type vhosts: :class:`list` of type `~obj.Vhost`
+
+    :returns: Display tuple - ('code', tag')
+    :rtype: `tuple`
+
+    """
+    choices = []
+    for vhost in vhosts:
+        if vhost.names == 1:
+            disp_name = next(iter(vhost.names))
+        elif vhost.names == 0:
+            disp_name = ""
+        else:
+            disp_name = "Multiple Names"
+
+        choices.append(
+            "%s | %s | %s | %s" % (
+                os.path.basename(vhost.filep),
+                disp_name,
+                "HTTPS" if vhost.ssl,
+                "Enabled" if vhost.enabled)
+        )
+
+    code, tag = zope.component.getUtility(interfaces.IDisplay).menu(
+        "We were unable to find a vhost with a Servername or Address of %s."
+        "Which virtual host would you like to choose?" % domain,
+        choices, help_label="More Info", ok_label="Select")
+
+    return code, tag
+
+
+def _more_info_vhost(vhost):
+    zope.component.getUtility(interfaces.IDisplay).notification(
+        "Virtual Host Information:{0}{1}".format(
+            os.linesep, str(vhost)),
+        height=display_util.HEIGHT)

letsencrypt_apache/dvsni.py

@@ -57,12 +57,6 @@ class ApacheDvsni(common.Dvsni):
         default_addr = "*:443"
         for achall in self.achalls:
             vhost = self.configurator.choose_vhost(achall.domain)
-            if vhost is None:
-                logging.error(
-                    "No vhost exists with servername or alias of: %s. "
-                    "No _default_:443 vhost exists. Please specify servernames "
-                    "in the Apache config", achall.domain)
-                return None
 
             # TODO - @jdkasten review this code to make sure it makes sense
             self.configurator.make_server_sni_ready(vhost, default_addr)

letsencrypt_apache/obj.py

@@ -32,12 +32,12 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
 
     def __str__(self):
         addr_str = ", ".join(str(addr) for addr in self.addrs)
-        return ("file: %s\n"
+        return ("File: %s\n"
-                "vh_path: %s\n"
+                "Vhost path: %s\n"
-                "addrs: %s\n"
+                "Addresses: %s\n"
-                "names: %s\n"
+                "Names: %s\n"
-                "ssl: %s\n"
+                "TLS: %s\n"
-                "enabled: %s" % (self.filep, self.path, addr_str,
+                "Enabled: %s" % (self.filep, self.path, addr_str,
                                  self.names, self.ssl, self.enabled))
 
     def __eq__(self, other):

letsencrypt_apache/tests/display_ops_test.py

@@ -0,0 +1,41 @@
+"""Test letsencrypt_apache.display_ops."""
+import unittest
+
+import mock
+
+from letsencrypt_apache.tests import util
+
+from letsencrypt.display import util as display_util
+
+
+class SelectVhostTest(unittest.TestCase):
+    """Tests for letsencrypt_apache.display_ops.select_vhost."""
+
+    def setUp(self):
+        self.base_dir = "/example_path"
+        self.vhosts = util.get_vh_truth(
+            self.base_dir, "debian_apache_2_4/two_vhost_80")
+
+    @classmethod
+    def _call(cls, vhosts):
+        from letsencrypt_apache.display_ops import select_vhost
+        select_vhost("example.com", vhosts)
+
+    @mock.patch("letsencrypt_apache.display_ops.zope.component.getUtility")
+    def test_successful_choice(self, mock_util):
+        mock_util().menu.return_value = (display_util.OK, 1)
+        self.assertEqual(self.vhosts[1], self._call(self.vhosts))
+
+    @mock.patch("letsencrypt_apache.display_ops.zope.component.getUtility")
+    def test_more_info_cancel(self, mock_util):
+        mock_util().menu.side_effect = [
+            (display_util.HELP, 1),
+            (display_util.HELP, 0),
+            (display_util.CANCEL, -1),
+        ]
+
+        self.assertEqual(None, self._call())
+        self.assertEqual(mock_util().notification.call_count, 2)
+
+    def test_no_vhosts(self):
+        self.assertEqual(self._call([]), None)